fix(telemetry): address parallel-review findings on consent PR

Author: edelauna

PRIVACY.md

@@ -39,7 +39,11 @@ go—and, importantly, where they don't.
   some jurisdictions; we use it only for product analytics and error grouping.
   We retain telemetry only as long as needed for product analytics and
   debugging. This PostHog-based telemetry does **not** collect your code or AI
-  prompts, and you can opt out at any time through the settings.
+  prompts. Telemetry is on by default. To turn it off, choose "Disabled" in
+  the settings or in the notice shown on first use; closing that notice
+  without a choice leaves telemetry on. Telemetry also follows VS Code's own
+  global telemetry setting: if you turn that off, Zoo Code stops sending
+  telemetry right away, even if the Zoo Code setting says "Enabled."
 - **Marketplace Requests**: When you browse or search the Marketplace for Model
   Configuration Profiles (MCPs) or Custom Modes, Zoo Code makes a secure API
   call to Zoo Code's backend servers to retrieve listing information. These

src/__tests__/extension.spec.ts

@@ -84,6 +84,7 @@ const mockTelemetryServiceInstance = {
 vi.mock("@roo-code/telemetry", () => ({
 	TelemetryService: {
 		createInstance: vi.fn().mockReturnValue(mockTelemetryServiceInstance),
+		hasInstance: vi.fn().mockReturnValue(true),
 		get instance() {
 			return mockTelemetryServiceInstance
 		},
@@ -461,5 +462,42 @@ describe("extension.ts", () => {
 
 			setTerminalProfileSpy.mockRestore()
 		})
+
+		// Review finding: every other TelemetryService call site touched by this PR checks
+		// hasInstance() first; deactivate()'s shutdown call didn't. Not a crash today (the mock
+		// always resolves), but TelemetryService.instance throws for real if no instance exists,
+		// so the guard keeps this call site consistent with the rest of the file.
+		test("does not touch TelemetryService.instance when no instance exists", async () => {
+			const { TelemetryService } = await import("@roo-code/telemetry")
+			const { Terminal } = await import("../integrations/terminal/Terminal")
+			const { TerminalRegistry } = await import("../integrations/terminal/TerminalRegistry")
+
+			const setTerminalProfileSpy = vi.spyOn(Terminal, "setTerminalProfile")
+
+			const { activate, deactivate } = await import("../extension")
+			await activate(mockContext)
+
+			// Flip to false only after activate() completes, so this only exercises
+			// deactivate()'s own guard rather than any hasInstance() check during activation.
+			vi.mocked(TelemetryService.hasInstance).mockReturnValue(false)
+
+			// Model the real singleton failure mode: TelemetryService.instance throws when no
+			// instance exists. If deactivate()'s hasInstance() guard were ever removed, this
+			// throw would surface instead of the assertion below silently passing regardless.
+			const instanceGetterSpy = vi.spyOn(TelemetryService, "instance", "get").mockImplementation(() => {
+				throw new Error("TelemetryService not initialized")
+			})
+
+			await expect(deactivate()).resolves.toBeUndefined()
+
+			expect(instanceGetterSpy).not.toHaveBeenCalled()
+			expect(mockTelemetryServiceInstance.shutdown).not.toHaveBeenCalled()
+			expect(setTerminalProfileSpy).toHaveBeenCalledWith(undefined)
+			expect(TerminalRegistry.cleanup).toHaveBeenCalledTimes(1)
+
+			instanceGetterSpy.mockRestore()
+
+			setTerminalProfileSpy.mockRestore()
+		})
 	})
 })

src/core/webview/__tests__/webviewMessageHandler.spec.ts

@@ -1930,6 +1930,37 @@ describe("webviewMessageHandler - telemetrySetting", () => {
 		expect(calls.at(-1)).toEqual([false])
 	})
 
+	// Review finding: webviewDidLaunch's queued telemetry update wasn't awaited by the
+	// "webviewDidLaunch" case, so a thrown error inside it was only ever caught by a later,
+	// unrelated queue link's leading .catch(() => undefined) -- silently swallowed rather than
+	// logged. Now awaited with its own .catch that logs via provider.log.
+	it("logs an error via provider.log if the queued telemetry init throws on launch", async () => {
+		const { TelemetryService } = await import("@roo-code/telemetry")
+		vi.mocked(TelemetryService.hasInstance).mockReturnValue(true)
+
+		vi.mocked(mockClineProvider.contextProxy.getValue).mockImplementation((key: string) => {
+			if (key === "telemetrySetting") {
+				throw new Error("contextProxy read failed")
+			}
+			return undefined
+		})
+		vi.mocked(mockClineProvider.customModesManager.getCustomModes).mockResolvedValue([])
+		const providerForLaunch = mockClineProvider as unknown as {
+			getMcpHub: ReturnType<typeof vi.fn>
+			providerSettingsManager: { listConfig: ReturnType<typeof vi.fn> }
+			getStateToPostToWebview: ReturnType<typeof vi.fn>
+		}
+		providerForLaunch.getMcpHub = vi.fn().mockReturnValue(undefined)
+		providerForLaunch.providerSettingsManager = { listConfig: vi.fn().mockResolvedValue(undefined) }
+		providerForLaunch.getStateToPostToWebview = vi.fn().mockResolvedValue({ telemetrySetting: "unset" })
+
+		await webviewMessageHandler(mockClineProvider, { type: "webviewDidLaunch" })
+
+		expect(mockClineProvider.log).toHaveBeenCalledWith(
+			expect.stringContaining("Error initializing telemetry state on launch"),
+		)
+	})
+
 	// CodeRabbit finding: webviewDidLaunch's queued telemetry update called
 	// TelemetryService.instance directly, unlike the "telemetrySetting" case a few lines
 	// below which checks hasInstance() first. If webviewDidLaunch fires before the service

src/core/webview/webviewMessageHandler.ts

@@ -669,6 +669,12 @@ export const webviewMessageHandler = async (
 					)
 				})
 
+			await telemetrySettingQueue.catch((error) =>
+				provider.log(
+					`Error initializing telemetry state on launch: ${error instanceof Error ? error.message : String(error)}`,
+				),
+			)
+
 			provider.isViewLaunched = true
 			break
 		case "newTask":

src/extension.ts

@@ -413,12 +413,14 @@ export async function deactivate() {
 
 	await McpServerManager.cleanup(extensionContext)
 
-	try {
-		await TelemetryService.instance.shutdown()
-	} catch (error) {
-		outputChannel.appendLine(
-			`Failed to shut down telemetry service: ${error instanceof Error ? error.message : String(error)}`,
-		)
+	if (TelemetryService.hasInstance()) {
+		try {
+			await TelemetryService.instance.shutdown()
+		} catch (error) {
+			outputChannel.appendLine(
+				`Failed to shut down telemetry service: ${error instanceof Error ? error.message : String(error)}`,
+			)
+		}
 	}
 
 	Terminal.setTerminalProfile(undefined)