feat(telemetry): TPA-scanner stats (schema v8) + tray menu alignment QA

docs/features/telemetry.md

@@ -4,7 +4,7 @@ MCPProxy collects anonymous usage statistics to help improve the product. This p
 
 ## What is collected
 
-MCPProxy sends a **daily heartbeat** containing only aggregate, non-identifying information. The current schema is **version 7** (`schema_version: 7` in the JSON payload); the schema is forward-compatible so older consumers simply ignore fields they don't recognize.
+MCPProxy sends a **daily heartbeat** containing only aggregate, non-identifying information. The current schema is **version 8** (`schema_version: 8` in the JSON payload); the schema is forward-compatible so older consumers simply ignore fields they don't recognize.
 
 | Field | Example | Purpose |
 |-------|---------|---------|
@@ -32,6 +32,8 @@ MCPProxy sends a **daily heartbeat** containing only aggregate, non-identifying
 | `active_days_30d` | `5` | Distinct UTC days with process activity in the trailing 30 days (schema v7). Only the count — never the per-day breakdown |
 | `previous_shutdown` | `clean` | How the previous process instance ended — fixed enum `clean` / `crash`, absent on first run (schema v7) |
 | `last_error_code` | `MCPX_DOCKER_CLI_NOT_FOUND` | Most recent stable `MCPX_*` diagnostic code (schema v7). Enum code only, never error text |
+| `tpa_scanner` | `{"scans_completed":4,"scans_failed":0,"scans_with_findings":1,"findings":{"high":2}}` | Security/TPA scanner activity (schema v8) — counts only, keyed by the fixed severity enum. Omitted entirely when no scan ran |
+| `feature_flags.deep_scan_enabled` | `false` | Whether the opt-in deep-scan layer is turned on (schema v8) |
 
 The `server_protocol_counts` map uses a **fixed enum of keys** (`stdio`, `http`, `sse`, `streamable_http`, `auto`) — server names and URLs are never included. Unknown or misconfigured protocol values are bucketed into `auto`.
 
@@ -132,6 +134,30 @@ You can inspect exactly what would be sent — including every v7 field — with
 mcpproxy telemetry show-payload
 ```
 
+## Schema v8 — security-scanner stats
+
+Schema v8 adds two purely **additive** signals so we can see whether the TPA / security scanner actually runs in the fleet, whether it fails, and whether it finds anything — without learning *what* it found or *where*.
+
+| Field | Type | When it is set | Privacy rationale |
+|-------|------|----------------|-------------------|
+| `tpa_scanner.scans_completed` | non-negative integer | Terminal, successful **scan jobs** since the last accepted heartbeat | A count of our own scan runs |
+| `tpa_scanner.scans_failed` | non-negative integer | Terminal **scan job** failures in the same window | The scanner id, the server, and the error text are never accepted by the counter API |
+| `tpa_scanner.scans_with_findings` | non-negative integer | Subset of `scans_completed` that produced at least one finding | Tells "scanner is running" apart from "scanner is finding things" |
+| `tpa_scanner.findings` | map, **fixed enum keys only** (`critical`/`high`/`medium`/`low`/`info`) → non-negative integer | Per-severity finding totals across the window. Severities with a zero total are omitted | Severity is a five-value enum; rule ids, finding titles, tool names, and file paths are dropped before they reach the counter |
+| `feature_flags.deep_scan_enabled` | boolean | The `security.deep_scan.enabled` master switch | A single boolean about our own config |
+
+**Unit of measure — one non-deep-scan (Pass 1) scan job.** Every `scans_*` counter counts *scan jobs*, not scanner invocations and not passes:
+
+- a job that runs five scanners counts **once**, however many of them fail — `scans_failed` counts failed jobs (all scanners failed), and a job that loses some scanners but still completes counts only in `scans_completed`;
+- the **Pass-2 deep supply-chain audit** that deep scan auto-starts after Pass 1 is **not counted**. Counting it would report ~2× the scans for exactly the deep-scan cohort that `feature_flags.deep_scan_enabled` exists to compare against everyone else;
+- **dry-run** jobs are not counted.
+
+The decision lives in the scanner package (`scanCallbackAdapter.countsForTelemetry` in `internal/security/scanner/service.go`), which is the only layer that knows a job's pass and dry-run status; it calls the single-purpose `EmitSecurityScanTelemetry` emitter hook, implemented on `Runtime` (`internal/runtime/event_bus.go`) as the only caller of the counter API. The UI-facing scan events (`EmitSecurityScanCompleted` / `EmitSecurityScanFailed`) deliberately record nothing — they fire per scanner and per pass.
+
+The whole `tpa_scanner` object is **omitted** when every counter is zero, so an install that never scans emits a payload shape-identical to v7. The anonymity scanner (`internal/telemetry/anonymity.go`, rule `v8_field_invalid`) re-asserts the contract on the serialized payload before every send: whitelisted keys, non-negative integers, and severity-enum keys only — a producer-side regression that leaked a server name or rule id as a map key would block the heartbeat rather than transmit it.
+
+**Never transmitted**: the scanned server's name, the scanner id, rule ids, finding titles or descriptions, matched content, file paths, and scan error messages.
+
 ## One-time opt-out signal
 
 When telemetry transitions from **enabled to disabled** (via the CLI, the config

internal/httpapi/telemetry_payload_test.go

@@ -127,7 +127,9 @@ func TestHandleGetTelemetryPayload_RendersV7Fields(t *testing.T) {
 	require.True(t, resp.Success)
 	require.NotNil(t, resp.Data)
 
-	assert.Equal(t, float64(7), resp.Data["schema_version"])
+	// Tracks telemetry.SchemaVersion — v8 added the tpa_scanner block; the
+	// v7 fields below must keep rendering regardless (FR-014: additive only).
+	assert.Equal(t, float64(telemetry.SchemaVersion), resp.Data["schema_version"])
 	assert.Equal(t, true, resp.Data["wizard_shown"])
 	assert.Equal(t, "completed_external", resp.Data["wizard_connect_step"])
 	assert.Equal(t, float64(1), resp.Data["web_ui_opened"])

internal/runtime/event_bus.go

@@ -661,6 +661,24 @@ func (r *Runtime) EmitSecurityScanFailed(serverName, _, errMsg string) {
 	r.publishScanSettled(serverName, "failed", nil, errMsg)
 }
 
+// EmitSecurityScanTelemetry is the sole producer of the schema-v8 TPA scanner
+// counters. The scanner package calls it exactly once per terminal, real
+// (non-dry-run) Pass-1 scan JOB — never per scanner and never for the Pass-2
+// deep supply-chain audit — so scans_completed/scans_failed count scans, not
+// scanner invocations or passes. See scanCallbackAdapter.countsForTelemetry.
+//
+// Only counts cross this boundary: the server name, the scanner id, and the
+// error text are not parameters at all, and the registry drops any severity key
+// outside the fixed enum. Nil-safe: telemetry may be disabled or not yet
+// initialized.
+func (r *Runtime) EmitSecurityScanTelemetry(completed bool, findingsBySeverity map[string]int) {
+	if completed {
+		telemetry.RecordTPAScanCompletedOn(r.TelemetryRegistry(), findingsBySeverity)
+		return
+	}
+	telemetry.RecordTPAScanFailedOn(r.TelemetryRegistry())
+}
+
 // publishScanSettled emits the single debounced terminal scan event.
 func (r *Runtime) publishScanSettled(serverName, status string, findingsSummary map[string]int, errMsg string) {
 	payload := map[string]any{

internal/runtime/event_bus_tpa_telemetry_test.go

@@ -0,0 +1,105 @@
+package runtime
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"go.uber.org/zap"
+
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/config"
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/telemetry"
+)
+
+// newTPATelemetryRuntime builds the minimal Runtime seam used by the scan
+// notification tests, plus a real telemetry service so the counter registry is
+// reachable through Runtime.TelemetryRegistry().
+func newTPATelemetryRuntime(t *testing.T) *Runtime {
+	t.Helper()
+	t.Setenv("DO_NOT_TRACK", "")
+	t.Setenv("CI", "")
+	t.Setenv("MCPPROXY_TELEMETRY", "")
+
+	rt := &Runtime{
+		logger:    zap.NewNop(),
+		eventSubs: make(map[chan Event]struct{}),
+	}
+	rt.telemetryService = telemetry.New(&config.Config{}, "", "v0.0.0-test", "personal", zap.NewNop())
+	return rt
+}
+
+// TestEmitSecurityScanTelemetry_RecordsCounters asserts the schema-v8 hook:
+// each call from the scanner's job-level seam increments the anonymous counters
+// — completions, failures, findings-by-severity — and nothing else.
+func TestEmitSecurityScanTelemetry_RecordsCounters(t *testing.T) {
+	rt := newTPATelemetryRuntime(t)
+	rt.scanNotify = newScanNotifyDebouncer(rt, 10*time.Millisecond)
+
+	rt.EmitSecurityScanTelemetry(true, map[string]int{"high": 2, "low": 1})
+	rt.EmitSecurityScanTelemetry(true, nil)
+	rt.EmitSecurityScanTelemetry(false, nil)
+
+	reg := rt.TelemetryRegistry()
+	require.NotNil(t, reg, "telemetry registry must be reachable from the runtime")
+	snap := reg.Snapshot()
+
+	assert.Equal(t, int64(2), snap.TPAScansCompleted)
+	assert.Equal(t, int64(1), snap.TPAScansFailed)
+	assert.Equal(t, int64(1), snap.TPAScansWithFindings)
+	assert.Equal(t, int64(2), snap.TPAFindings["high"])
+	assert.Equal(t, int64(1), snap.TPAFindings["low"])
+	assert.Equal(t, int64(0), snap.TPAFindings["critical"])
+
+	// Privacy: only the fixed severity enum reaches the registry — no server
+	// names, scanner ids, or error text.
+	for k := range snap.TPAFindings {
+		assert.True(t, telemetry.IsTPASeverity(k), "unexpected findings key %q", k)
+	}
+}
+
+// TestEmitSecurityScanEvents_DoNotRecordTelemetry pins the fix for the
+// over-counting bug: the UI-facing scan events fire per SCANNER (failures) and
+// per PASS (completions), so they must record nothing. Only the dedicated
+// job-level hook feeds the counters.
+func TestEmitSecurityScanEvents_DoNotRecordTelemetry(t *testing.T) {
+	rt := newTPATelemetryRuntime(t)
+	rt.scanNotify = newScanNotifyDebouncer(rt, 10*time.Millisecond)
+
+	// A single scan whose three scanners all fail, plus the Pass-1 and Pass-2
+	// completions that follow a deep scan.
+	rt.EmitSecurityScanFailed("my-private-server", "tpa-descriptions", "boom: /Users/algis/secret")
+	rt.EmitSecurityScanFailed("my-private-server", "trivy", "boom")
+	rt.EmitSecurityScanFailed("my-private-server", "cisco", "boom")
+	rt.EmitSecurityScanCompleted("my-private-server", map[string]int{"high": 2})
+	rt.EmitSecurityScanCompleted("my-private-server", map[string]int{"critical": 1})
+
+	reg := rt.TelemetryRegistry()
+	require.NotNil(t, reg)
+	snap := reg.Snapshot()
+
+	assert.Equal(t, int64(0), snap.TPAScansCompleted, "UI completion events must not record telemetry")
+	assert.Equal(t, int64(0), snap.TPAScansFailed, "per-scanner failures must not record telemetry")
+	assert.Equal(t, int64(0), snap.TPAScansWithFindings)
+	for sev, n := range snap.TPAFindings {
+		assert.Equal(t, int64(0), n, "severity %q must be untouched by UI events", sev)
+	}
+}
+
+// TestEmitSecurityScan_NilTelemetryIsSafe pins the nil-safe path: a Runtime
+// without a telemetry service (telemetry disabled or not yet initialized) must
+// still emit scan events without panicking.
+func TestEmitSecurityScan_NilTelemetryIsSafe(t *testing.T) {
+	rt := &Runtime{
+		logger:    zap.NewNop(),
+		eventSubs: make(map[chan Event]struct{}),
+	}
+	require.Nil(t, rt.TelemetryRegistry())
+
+	assert.NotPanics(t, func() {
+		rt.EmitSecurityScanCompleted("srv", map[string]int{"high": 1})
+		rt.EmitSecurityScanFailed("srv", "tpa-descriptions", "err")
+		rt.EmitSecurityScanTelemetry(true, map[string]int{"high": 1})
+		rt.EmitSecurityScanTelemetry(false, nil)
+	})
+}

internal/security/scanner/scan_telemetry_test.go

@@ -0,0 +1,111 @@
+package scanner
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// telemetrySamples extracts the anonymous scan-telemetry samples recorded by the
+// mock emitter, in order. Each sample is (completed, findings).
+func telemetrySamples(t *testing.T, em *mockEmitter) []mockEvent {
+	t.Helper()
+	em.mu.Lock()
+	defer em.mu.Unlock()
+	var out []mockEvent
+	for _, ev := range em.events {
+		if ev.eventType == "scan_telemetry" {
+			out = append(out, ev)
+		}
+	}
+	return out
+}
+
+// TestScanTelemetry_Pass1CompletionRecordsOnce pins the unit of measure: one
+// terminal Pass-1 job produces exactly one completed sample carrying the
+// aggregated per-severity counts (and nothing else).
+func TestScanTelemetry_Pass1CompletionRecordsOnce(t *testing.T) {
+	svc, _, em := newTestService(t)
+	adapter := &scanCallbackAdapter{service: svc, scanPass: ScanPassSecurityScan}
+
+	job := &ScanJob{ID: "job-1", ServerName: "my-private-server", ScanPass: ScanPassSecurityScan}
+	reports := []*ScanReport{
+		{Findings: []ScanFinding{{Severity: "high"}, {Severity: "high"}}},
+		{Findings: []ScanFinding{{Severity: "low"}}},
+	}
+	adapter.OnScanCompleted(job, reports)
+
+	samples := telemetrySamples(t, em)
+	require.Len(t, samples, 1, "one Pass-1 job ⇒ exactly one telemetry sample")
+	assert.Equal(t, true, samples[0].data["completed"])
+	assert.Equal(t, map[string]int{"high": 2, "low": 1}, samples[0].data["findings"])
+}
+
+// TestScanTelemetry_Pass2DoesNotRecord is the regression guard for the
+// double-counting bug: deep scan auto-starts a Pass-2 supply-chain job after
+// every Pass 1, so counting Pass 2 would report ~2x scans for exactly the
+// deep-scan cohort the counters exist to compare.
+func TestScanTelemetry_Pass2DoesNotRecord(t *testing.T) {
+	svc, _, em := newTestService(t)
+	adapter := &scanCallbackAdapter{service: svc, scanPass: ScanPassSupplyChainAudit}
+
+	job := &ScanJob{ID: "job-2", ServerName: "my-private-server", ScanPass: ScanPassSupplyChainAudit}
+	adapter.OnScanCompleted(job, []*ScanReport{{Findings: []ScanFinding{{Severity: "critical"}}}})
+	adapter.OnScanFailed(job, errors.New("all scanners failed"))
+
+	assert.Empty(t, telemetrySamples(t, em), "Pass 2 must never record scan telemetry")
+}
+
+// TestScanTelemetry_PerScannerFailuresDoNotRecord pins the other half of the
+// fix: OnScannerFailed fires once PER FAILING SCANNER, so it must not record.
+// A job with three failing scanners is still one failed scan, counted once by
+// the job-level OnScanFailed.
+func TestScanTelemetry_PerScannerFailuresDoNotRecord(t *testing.T) {
+	svc, _, em := newTestService(t)
+	adapter := &scanCallbackAdapter{service: svc, scanPass: ScanPassSecurityScan}
+
+	job := &ScanJob{ID: "job-3", ServerName: "my-private-server", ScanPass: ScanPassSecurityScan}
+	adapter.OnScannerFailed(job, "test-scanner", errors.New("boom"))
+	adapter.OnScannerFailed(job, "scanner-b", errors.New("boom"))
+	adapter.OnScannerFailed(job, "scanner-c", errors.New("boom"))
+
+	assert.Empty(t, telemetrySamples(t, em), "per-scanner failures must not record scan telemetry")
+
+	// The job-level terminal failure records exactly one failed sample.
+	adapter.OnScanFailed(job, errors.New("all scanners failed"))
+	samples := telemetrySamples(t, em)
+	require.Len(t, samples, 1)
+	assert.Equal(t, false, samples[0].data["completed"])
+	assert.Nil(t, samples[0].data["findings"], "a failure sample carries no findings")
+}
+
+// TestScanTelemetry_PartialFailureCountsOnlyAsCompletion pins the mixed case:
+// a job that loses some scanners but still completes is a completion, not both
+// a completion and a failure.
+func TestScanTelemetry_PartialFailureCountsOnlyAsCompletion(t *testing.T) {
+	svc, _, em := newTestService(t)
+	adapter := &scanCallbackAdapter{service: svc, scanPass: ScanPassSecurityScan}
+
+	job := &ScanJob{ID: "job-4", ServerName: "my-private-server", ScanPass: ScanPassSecurityScan}
+	adapter.OnScannerFailed(job, "test-scanner", errors.New("boom"))
+	adapter.OnScanCompleted(job, []*ScanReport{{Findings: []ScanFinding{{Severity: "medium"}}}})
+
+	samples := telemetrySamples(t, em)
+	require.Len(t, samples, 1)
+	assert.Equal(t, true, samples[0].data["completed"])
+}
+
+// TestScanTelemetry_DryRunDoesNotRecord: dry-run jobs do not affect quarantine
+// state and are not real scans, so they stay out of the counters.
+func TestScanTelemetry_DryRunDoesNotRecord(t *testing.T) {
+	svc, _, em := newTestService(t)
+	adapter := &scanCallbackAdapter{service: svc, scanPass: ScanPassSecurityScan}
+
+	job := &ScanJob{ID: "job-5", ServerName: "my-private-server", ScanPass: ScanPassSecurityScan, DryRun: true}
+	adapter.OnScanCompleted(job, []*ScanReport{{Findings: []ScanFinding{{Severity: "high"}}}})
+	adapter.OnScanFailed(job, errors.New("all scanners failed"))
+
+	assert.Empty(t, telemetrySamples(t, em), "dry-run jobs must not record scan telemetry")
+}

internal/security/scanner/service.go

@@ -63,6 +63,18 @@ type EventEmitter interface {
 	// changed (e.g., background pull started/finished/failed) so the web UI
 	// can refresh its scanner list without polling.
 	EmitSecurityScannerChanged(scannerID, status, errMsg string)
+	// EmitSecurityScanTelemetry records ONE anonymous, counts-only sample for a
+	// terminal scan JOB. It is deliberately separate from the UI-facing
+	// EmitSecurityScanCompleted/EmitSecurityScanFailed events, which fire per
+	// SCANNER and per PASS and would therefore over-count: the unit measured
+	// here is "one non-deep-scan (Pass 1) scan job".
+	//
+	// Only the scanner package knows a job's pass and dry-run status, so the
+	// decision of whether a job counts lives here (scanCallbackAdapter) rather
+	// than in the emitter. Implementations must accept counts only:
+	// findingsBySeverity is severity -> count and carries no server name,
+	// scanner id, rule id, or free text.
+	EmitSecurityScanTelemetry(completed bool, findingsBySeverity map[string]int)
 }
 
 // NoopEmitter is a no-op implementation of EventEmitter
@@ -74,6 +86,7 @@ func (n *NoopEmitter) EmitSecurityScanCompleted(string, map[string]int)     {}
 func (n *NoopEmitter) EmitSecurityScanFailed(string, string, string)        {}
 func (n *NoopEmitter) EmitSecurityIntegrityAlert(string, string, string)    {}
 func (n *NoopEmitter) EmitSecurityScannerChanged(string, string, string)    {}
+func (n *NoopEmitter) EmitSecurityScanTelemetry(bool, map[string]int)       {}
 
 // ServerInfoProvider resolves server configuration for auto-source resolution
 type ServerInfoProvider interface {
@@ -831,6 +844,17 @@ type scanCallbackAdapter struct {
 	serverInfo *ServerInfo // Cached server info for pass 2 auto-start
 }
 
+// countsForTelemetry reports whether this job is the unit the anonymous
+// schema-v8 scanner counters measure: one real (non-dry-run) Pass-1 scan job.
+//
+// Pass 2 (the deep supply-chain audit auto-started after Pass 1) is excluded
+// deliberately — counting it would double the reported scan volume of exactly
+// the deep-scan cohort the counters exist to compare. Dry-run jobs are excluded
+// because they do not affect quarantine state and are not real scans.
+func (a *scanCallbackAdapter) countsForTelemetry(job *ScanJob) bool {
+	return job != nil && a.scanPass == ScanPassSecurityScan && !job.DryRun
+}
+
 func (a *scanCallbackAdapter) OnScanStarted(job *ScanJob) {
 	_ = a.service.storage.SaveScanJob(job)
 	a.service.emit().EmitSecurityScanStarted(job.ServerName, job.Scanners, job.ID)
@@ -866,6 +890,11 @@ func (a *scanCallbackAdapter) OnScanCompleted(job *ScanJob, reports []*ScanRepor
 		}
 	}
 	a.service.emit().EmitSecurityScanCompleted(job.ServerName, summary)
+	// Anonymous telemetry (schema v8): one sample per real Pass-1 job, NOT per
+	// scanner and NOT for the Pass-2 deep audit.
+	if a.countsForTelemetry(job) {
+		a.service.emit().EmitSecurityScanTelemetry(true, summary)
+	}
 	// Cleanup auto-resolved source directory
 	if a.cleanup != nil {
 		a.cleanup()
@@ -890,6 +919,13 @@ func (a *scanCallbackAdapter) OnScanFailed(job *ScanJob, err error) {
 	_ = a.service.storage.SaveScanJob(job)
 	// Invalidate cached summary
 	a.service.invalidateScanSummaryCache(job.ServerName)
+	// Anonymous telemetry (schema v8): a FAILED job counts once here. The
+	// per-scanner OnScannerFailed path must not record — a job with three
+	// failing scanners is still one failed scan, and a job that fails some
+	// scanners but still completes is a completion, not a failure.
+	if a.countsForTelemetry(job) {
+		a.service.emit().EmitSecurityScanTelemetry(false, nil)
+	}
 	// Cleanup auto-resolved source directory
 	if a.cleanup != nil {
 		a.cleanup()