From 7f2c234150d256cefdcb35ed28ed207d3243fa36 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 5 Aug 2026 13:19:35 +0300 Subject: [PATCH 1/4] feat(telemetry): anonymous TPA-scanner stats in heartbeat (schema v8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New tpa_scanner payload block: scans completed/failed/with-findings and findings by severity (fixed 5-key enum), plus feature_flags.deep_scan_enabled. Counting unit is one Pass-1, non-dry-run scan job — deep-scan Pass-2 jobs and per-scanner failures are explicitly excluded so the deep-scan cohort comparison is not skewed. Recording is gated in the scanner package via a new EventEmitter.EmitSecurityScanTelemetry seam; the UI-facing scan events no longer touch telemetry. Privacy: counts and enum keys only — server names, scanner ids, rule ids, finding titles and paths structurally cannot reach the payload; a new v8_field_invalid anonymity rule drops any malformed payload before send. Counters live in CounterRegistry under one lock (snapshot can never show findings without scans) and reset only after an accepted heartbeat. --- docs/features/telemetry.md | 28 +- internal/runtime/event_bus.go | 18 + .../runtime/event_bus_tpa_telemetry_test.go | 105 +++++ .../security/scanner/scan_telemetry_test.go | 111 +++++ internal/security/scanner/service.go | 36 ++ internal/security/scanner/service_test.go | 9 + internal/telemetry/anonymity.go | 100 ++++- internal/telemetry/feature_flags.go | 12 + internal/telemetry/payload_privacy_test.go | 2 +- internal/telemetry/payload_v2_test.go | 6 +- internal/telemetry/payload_v7_test.go | 20 +- internal/telemetry/registry.go | 129 ++++++ internal/telemetry/telemetry.go | 39 +- internal/telemetry/telemetry_test.go | 14 +- internal/telemetry/tpa_scanner.go | 73 ++++ internal/telemetry/tpa_scanner_test.go | 407 ++++++++++++++++++ 16 files changed, 1085 insertions(+), 24 deletions(-) create mode 100644 internal/runtime/event_bus_tpa_telemetry_test.go create mode 100644 internal/security/scanner/scan_telemetry_test.go create mode 100644 internal/telemetry/tpa_scanner.go create mode 100644 internal/telemetry/tpa_scanner_test.go diff --git a/docs/features/telemetry.md b/docs/features/telemetry.md index c69210538..66397ef10 100644 --- a/docs/features/telemetry.md +++ b/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 diff --git a/internal/runtime/event_bus.go b/internal/runtime/event_bus.go index 28e38735b..8bc852339 100644 --- a/internal/runtime/event_bus.go +++ b/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{ diff --git a/internal/runtime/event_bus_tpa_telemetry_test.go b/internal/runtime/event_bus_tpa_telemetry_test.go new file mode 100644 index 000000000..18ab7475c --- /dev/null +++ b/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) + }) +} diff --git a/internal/security/scanner/scan_telemetry_test.go b/internal/security/scanner/scan_telemetry_test.go new file mode 100644 index 000000000..31e0171ed --- /dev/null +++ b/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") +} diff --git a/internal/security/scanner/service.go b/internal/security/scanner/service.go index b973aaca9..96f176b03 100644 --- a/internal/security/scanner/service.go +++ b/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() diff --git a/internal/security/scanner/service_test.go b/internal/security/scanner/service_test.go index 2c6fae3ce..091704617 100644 --- a/internal/security/scanner/service_test.go +++ b/internal/security/scanner/service_test.go @@ -315,6 +315,15 @@ func (e *mockEmitter) EmitSecurityScannerChanged(scannerID, status, errMsg strin }) } +func (e *mockEmitter) EmitSecurityScanTelemetry(completed bool, findingsBySeverity map[string]int) { + e.mu.Lock() + defer e.mu.Unlock() + e.events = append(e.events, mockEvent{ + eventType: "scan_telemetry", + data: map[string]interface{}{"completed": completed, "findings": findingsBySeverity}, + }) +} + // mockUnquarantiner records UnquarantineServer calls for test assertions. type mockUnquarantiner struct { mu sync.Mutex diff --git a/internal/telemetry/anonymity.go b/internal/telemetry/anonymity.go index 7b849d338..92b3d5e91 100644 --- a/internal/telemetry/anonymity.go +++ b/internal/telemetry/anonymity.go @@ -85,6 +85,10 @@ type anonymityScanEnvelope struct { ActiveDays30d *json.RawMessage `json:"active_days_30d"` PreviousShutdown *json.RawMessage `json:"previous_shutdown"` LastErrorCode *json.RawMessage `json:"last_error_code"` + + // Schema v8 structural check: the security-scanner sub-object must be + // counts-and-fixed-enum-keys only. + TPAScanner *json.RawMessage `json:"tpa_scanner"` } // v7FieldViolation builds the violation for a Spec 080 field that broke its @@ -112,6 +116,12 @@ func scanV7Bool(raw *json.RawMessage, field string) *AnonymityViolation { // scanV7NonNegativeInt asserts raw (if present) is a non-negative JSON // integer — no fractions, no strings, no null. func scanV7NonNegativeInt(raw *json.RawMessage, field string) *AnonymityViolation { + return scanNonNegativeInt(raw, field, v7FieldViolation) +} + +// scanNonNegativeInt is the shared non-negative-integer assertion. mkViolation +// tags the failure with the schema-version rule of the calling scan pass. +func scanNonNegativeInt(raw *json.RawMessage, field string, mkViolation func(field, reason string) *AnonymityViolation) *AnonymityViolation { if raw == nil { return nil } @@ -119,18 +129,18 @@ func scanV7NonNegativeInt(raw *json.RawMessage, field string) *AnonymityViolatio // number token so strings never masquerade as counters. trimmed := bytes.TrimSpace(*raw) if len(trimmed) == 0 || trimmed[0] == '"' { - return v7FieldViolation(field, "must be a number") + return mkViolation(field, "must be a number") } var n json.Number if err := json.Unmarshal(trimmed, &n); err != nil { - return v7FieldViolation(field, "must be a number") + return mkViolation(field, "must be a number") } i, err := n.Int64() if err != nil { - return v7FieldViolation(field, "must be a whole integer") + return mkViolation(field, "must be a whole integer") } if i < 0 { - return v7FieldViolation(field, "must be non-negative") + return mkViolation(field, "must be non-negative") } return nil } @@ -197,6 +207,80 @@ func scanV7Fields(env *anonymityScanEnvelope) *AnonymityViolation { return nil } +// v8FieldViolation builds the violation for a schema-v8 field that broke its +// documented shape (whitelisted keys, non-negative counts, fixed severity +// enum). +func v8FieldViolation(field, reason string) *AnonymityViolation { + return &AnonymityViolation{ + Rule: "v8_field_invalid", + Pattern: field, + Reason: fmt.Sprintf("v8 field %s %s", field, reason), + } +} + +// tpaScannerScalarKeys is the fixed set of non-negative-integer keys allowed +// in the tpa_scanner sub-object. +var tpaScannerScalarKeys = []string{"scans_completed", "scans_failed", "scans_with_findings"} + +// scanV8TPAScanner asserts the schema-v8 tpa_scanner sub-object (if present) +// carries counts and fixed enum keys ONLY: an object whose keys are +// whitelisted, whose scalars are non-negative integers, and whose findings map +// is keyed exclusively by the severity enum with non-negative integer values. +// This is the wire-form backstop for the producer-side filtering in +// CounterRegistry.RecordTPAScanCompleted — a regression there (e.g. a server +// name or rule id leaking in as a map key) is caught before transmit. +func scanV8TPAScanner(raw *json.RawMessage) *AnonymityViolation { + if raw == nil { + return nil + } + var obj map[string]json.RawMessage + if err := json.Unmarshal(*raw, &obj); err != nil { + return v8FieldViolation("tpa_scanner", "must be an object") + } + + allowed := make(map[string]struct{}, len(tpaScannerScalarKeys)+1) + for _, k := range tpaScannerScalarKeys { + allowed[k] = struct{}{} + } + allowed["findings"] = struct{}{} + for k := range obj { + if _, ok := allowed[k]; !ok { + return v8FieldViolation("tpa_scanner."+k, "is not a whitelisted key") + } + } + + for _, k := range tpaScannerScalarKeys { + v, ok := obj[k] + if !ok { + continue + } + msg := json.RawMessage(v) + if viol := scanNonNegativeInt(&msg, "tpa_scanner."+k, v8FieldViolation); viol != nil { + return viol + } + } + + rawFindings, ok := obj["findings"] + if !ok { + return nil + } + var findings map[string]json.RawMessage + if err := json.Unmarshal(rawFindings, &findings); err != nil { + return v8FieldViolation("tpa_scanner.findings", "must be an object") + } + for sev, v := range findings { + if !IsTPASeverity(sev) { + return v8FieldViolation("tpa_scanner.findings", + "carries a key outside the fixed severity enum") + } + msg := json.RawMessage(v) + if viol := scanNonNegativeInt(&msg, "tpa_scanner.findings."+sev, v8FieldViolation); viol != nil { + return viol + } + } + return nil +} + // ScanForPII scans a serialized telemetry payload (v3+) for PII leaks and // structural violations. Returns nil when the payload is clean; otherwise // returns an *AnonymityViolation. The returned error satisfies @@ -211,6 +295,9 @@ func scanV7Fields(env *anonymityScanEnvelope) *AnonymityViolation { // (wizard_shown), non-negative integers (web_ui_opened, // days_since_install, active_days_30d), or fixed enums // (wizard_connect_step, previous_shutdown, last_error_code = MCPX_*). +// 5. tpa_scanner (schema v8), if present, is not an object of whitelisted +// keys holding non-negative integer counts, with a findings map keyed +// exclusively by the fixed severity enum. // // The implementation never logs the payload — it only reports which rule // tripped and the offending pattern (a small literal). Callers should log at @@ -271,6 +358,11 @@ func ScanForPII(payloadJSON []byte) error { return v } + // Rule 5: schema-v8 tpa_scanner must be counts + fixed severity keys only. + if v := scanV8TPAScanner(env.TPAScanner); v != nil { + return v + } + return nil } diff --git a/internal/telemetry/feature_flags.go b/internal/telemetry/feature_flags.go index 0c973e151..16f707673 100644 --- a/internal/telemetry/feature_flags.go +++ b/internal/telemetry/feature_flags.go @@ -40,6 +40,13 @@ type FeatureFlagSnapshot struct { // Populated by the telemetry service at heartbeat time (the resolution is a // runtime concern) — mirrors DockerAvailable. DockerCLISource string `json:"docker_cli_source,omitempty"` + + // Schema v8: DeepScanEnabled is the opt-in deep-scan master switch + // (security.deep_scan.enabled). Set by BuildFeatureFlagSnapshot (pure, + // config-only) like DockerIsolationEnabled. It lets the dashboard read + // tpa_scanner scan volume against the population that actually turned the + // deep-scan layer on. + DeepScanEnabled bool `json:"deep_scan_enabled"` } // protocolKeys is the canonical fixed-enum set of protocol labels emitted by @@ -142,6 +149,11 @@ func BuildFeatureFlagSnapshot(cfg *config.Config) *FeatureFlagSnapshot { snap.DockerIsolationEnabled = cfg.DockerIsolation.Enabled } + // Schema v8: deep-scan master switch. IsDeepScanEnabled is nil-safe on + // both the SecurityConfig and its DeepScan block, so a config without the + // security block reports false. + snap.DeepScanEnabled = cfg.Security.IsDeepScanEnabled() + // Derive OAuth provider types from upstream server URLs. var providerTypes []string for _, srv := range cfg.Servers { diff --git a/internal/telemetry/payload_privacy_test.go b/internal/telemetry/payload_privacy_test.go index 4ec3b794b..46a4b63c7 100644 --- a/internal/telemetry/payload_privacy_test.go +++ b/internal/telemetry/payload_privacy_test.go @@ -144,7 +144,7 @@ func TestPayloadHasNoForbiddenSubstrings(t *testing.T) { // Sanity check: the payload should still contain the legitimate fields, // otherwise we've over-redacted. for _, required := range []string{ - `"schema_version":7`, + `"schema_version":8`, `"surface_requests"`, `"builtin_tool_calls"`, `"upstream_tool_call_count_bucket"`, diff --git a/internal/telemetry/payload_v2_test.go b/internal/telemetry/payload_v2_test.go index 8f5db58f7..b4e5c11ce 100644 --- a/internal/telemetry/payload_v2_test.go +++ b/internal/telemetry/payload_v2_test.go @@ -88,8 +88,8 @@ func TestHeartbeatPayloadV2Marshal(t *testing.T) { payload := svc.BuildPayload() - if payload.SchemaVersion != 7 { - t.Errorf("schema_version = %d, want 7", payload.SchemaVersion) + if payload.SchemaVersion != 8 { + t.Errorf("schema_version = %d, want 8", payload.SchemaVersion) } if payload.AnonymousID != "fixed-id" { t.Errorf("anonymous_id = %q", payload.AnonymousID) @@ -135,7 +135,7 @@ func TestHeartbeatPayloadV2Marshal(t *testing.T) { } js := string(data) for _, key := range []string{ - `"schema_version":7`, + `"schema_version":8`, `"surface_requests"`, `"builtin_tool_calls"`, `"upstream_tool_call_count_bucket":"11-100"`, diff --git a/internal/telemetry/payload_v7_test.go b/internal/telemetry/payload_v7_test.go index 519c785b7..824e17b4b 100644 --- a/internal/telemetry/payload_v7_test.go +++ b/internal/telemetry/payload_v7_test.go @@ -7,10 +7,16 @@ import ( "time" ) -// TestSchemaVersionIsV7 pins FR-014: the Spec 080 payload contract is v7. -func TestSchemaVersionIsV7(t *testing.T) { - if SchemaVersion != 7 { - t.Fatalf("SchemaVersion = %d, want 7 (Spec 080 FR-014)", SchemaVersion) +// TestSchemaVersionIsAtLeastV7 pins FR-014: the Spec 080 payload contract +// shipped at v7 and may only move forward. The current version is 8 (the +// additive tpa_scanner / deep_scan_enabled bump); a downgrade below 7 would +// drop the Spec 080 fields. +func TestSchemaVersionIsAtLeastV7(t *testing.T) { + if SchemaVersion < 7 { + t.Fatalf("SchemaVersion = %d, want >= 7 (Spec 080 FR-014)", SchemaVersion) + } + if SchemaVersion != 8 { + t.Fatalf("SchemaVersion = %d, want 8 (v8 tpa_scanner additions)", SchemaVersion) } } @@ -72,7 +78,7 @@ func TestPayloadV7_FullyPopulatedPassesScanner(t *testing.T) { } for _, required := range []string{ - `"schema_version":7`, + `"schema_version":8`, `"wizard_shown":true`, `"wizard_connect_step":"completed_external"`, `"web_ui_opened":3`, @@ -103,8 +109,8 @@ func TestPayloadV7_ZeroNewFieldsShapeCompatibleWithV6(t *testing.T) { } js := string(data) - if !strings.Contains(js, `"schema_version":7`) { - t.Errorf("expected schema_version:7 even on a zero-valued payload, got:\n%s", js) + if !strings.Contains(js, `"schema_version":8`) { + t.Errorf("expected schema_version:8 even on a zero-valued payload, got:\n%s", js) } for _, forbidden := range []string{ `"wizard_shown"`, diff --git a/internal/telemetry/registry.go b/internal/telemetry/registry.go index ee4ba99d6..4acfdf9be 100644 --- a/internal/telemetry/registry.go +++ b/internal/telemetry/registry.go @@ -115,6 +115,21 @@ type CounterRegistry struct { restEndpoints map[string]map[string]int64 // method+template -> status class -> count errorCategories map[ErrorCategory]int64 doctorChecks map[string]*DoctorCounts + + // Schema v8: TPA / security-scanner outcome counters. Counts only — the + // scanned server's name, the scanner id, the rule id, and the finding + // title NEVER reach the registry (the Record* methods do not accept them). + // + // These are plain int64 guarded by mu (NOT atomics) on purpose: the scalar + // counters and tpaFindings are always written together and read together, + // so a lock-free scalar would let Snapshot observe findings without the + // scan that produced them (or a scan with its findings not yet visible). + tpaScansCompleted int64 + tpaScansFailed int64 + tpaScansWithFindings int64 + // tpaFindings is keyed ONLY by the fixed severity enum (see + // tpaSeverityKeys); unknown keys are dropped by RecordTPAScanCompleted. + tpaFindings map[string]int64 } // NewCounterRegistry creates an empty registry. All counters start at zero. @@ -124,6 +139,7 @@ func NewCounterRegistry() *CounterRegistry { restEndpoints: make(map[string]map[string]int64), errorCategories: make(map[ErrorCategory]int64), doctorChecks: make(map[string]*DoctorCounts), + tpaFindings: make(map[string]int64), } } @@ -179,6 +195,24 @@ func RecordErrorOn(reg *CounterRegistry, c ErrorCategory) { reg.RecordError(c) } +// RecordTPAScanCompletedOn calls reg.RecordTPAScanCompleted(...) if reg is +// non-nil (schema v8). +func RecordTPAScanCompletedOn(reg *CounterRegistry, findingsBySeverity map[string]int) { + if reg == nil { + return + } + reg.RecordTPAScanCompleted(findingsBySeverity) +} + +// RecordTPAScanFailedOn calls reg.RecordTPAScanFailed() if reg is non-nil +// (schema v8). +func RecordTPAScanFailedOn(reg *CounterRegistry) { + if reg == nil { + return + } + reg.RecordTPAScanFailed() +} + // RecordBuiltinTool increments the counter for the named built-in tool. // Unknown names (i.e., upstream tool names) are silently dropped. func (r *CounterRegistry) RecordBuiltinTool(name string) { @@ -234,6 +268,51 @@ func (r *CounterRegistry) RecordError(c ErrorCategory) { r.mu.Unlock() } +// RecordTPAScanCompleted records one terminal, successful security scan +// (schema v8). findingsBySeverity is the per-severity finding count for that +// scan; only the fixed severity enum (critical/high/medium/low/info) is +// aggregated — any other key is silently dropped so scanner-specific or +// user-controlled strings can never inflate the payload's cardinality. +// +// A scan whose summary contains at least one POSITIVE count also increments +// the scans-with-findings counter. Nothing identifying the scanned server, the +// scanner, or the finding itself is accepted by this method. +func (r *CounterRegistry) RecordTPAScanCompleted(findingsBySeverity map[string]int) { + // Filter to the fixed enum first so the lock is held only for real work. + var filtered map[string]int64 + for sev, n := range findingsBySeverity { + if n <= 0 || !IsTPASeverity(sev) { + continue + } + if filtered == nil { + filtered = make(map[string]int64, len(tpaSeverityKeys)) + } + filtered[sev] += int64(n) + } + + // One critical section for the whole sample: a concurrent Snapshot must + // never see the findings without the scan, or vice versa. + r.mu.Lock() + defer r.mu.Unlock() + r.tpaScansCompleted++ + if len(filtered) == 0 { + return + } + r.tpaScansWithFindings++ + for sev, n := range filtered { + r.tpaFindings[sev] += n + } +} + +// RecordTPAScanFailed records one terminal security-scan failure (schema v8). +// The error message, scanner id, and server name are intentionally not +// accepted: only an aggregate count is recorded. +func (r *CounterRegistry) RecordTPAScanFailed() { + r.mu.Lock() + r.tpaScansFailed++ + r.mu.Unlock() +} + // RecordDoctorRun aggregates the structured doctor check results into the // registry's doctor counter. Each result increments either Pass or Fail for // its check name. @@ -270,6 +349,38 @@ type RegistrySnapshot struct { RESTEndpointCalls map[string]map[string]int64 `json:"rest_endpoint_calls"` ErrorCategoryCounts map[string]int64 `json:"error_category_counts"` DoctorChecks map[string]DoctorCounts `json:"doctor_checks"` + + // Schema v8: TPA / security-scanner outcome counters. TPAFindings always + // carries exactly the fixed severity keys (zeros included), mirroring the + // SurfaceCounts convention. + TPAScansCompleted int64 `json:"tpa_scans_completed"` + TPAScansFailed int64 `json:"tpa_scans_failed"` + TPAScansWithFindings int64 `json:"tpa_scans_with_findings"` + TPAFindings map[string]int64 `json:"tpa_findings"` +} + +// TPAScannerStats projects the schema-v8 scanner counters out of the snapshot, +// or returns nil when every counter is zero so the heartbeat can omit the +// sub-object entirely (same posture as Diagnostics). +func (s RegistrySnapshot) TPAScannerStats() *TPAScannerStats { + stats := &TPAScannerStats{ + ScansCompleted: s.TPAScansCompleted, + ScansFailed: s.TPAScansFailed, + ScansWithFindings: s.TPAScansWithFindings, + } + for sev, n := range s.TPAFindings { + if n == 0 { + continue + } + if stats.Findings == nil { + stats.Findings = make(map[string]int64, len(tpaSeverityKeys)) + } + stats.Findings[sev] = n + } + if stats.isZero() { + return nil + } + return stats } // Snapshot returns an immutable view of all counters. The registry is NOT @@ -282,6 +393,12 @@ func (r *CounterRegistry) Snapshot() RegistrySnapshot { RESTEndpointCalls: make(map[string]map[string]int64), ErrorCategoryCounts: make(map[string]int64), DoctorChecks: make(map[string]DoctorCounts), + TPAFindings: make(map[string]int64, len(tpaSeverityKeys)), + } + + // TPA findings: every severity key is always present, even if zero. + for _, sev := range tpaSeverityKeys { + snap.TPAFindings[sev] = 0 } // Surface counts: every key is always present, even if zero. @@ -308,6 +425,14 @@ func (r *CounterRegistry) Snapshot() RegistrySnapshot { for k, v := range r.doctorChecks { snap.DoctorChecks[k] = *v } + // TPA scalars are read under the same lock as tpaFindings so the sample is + // internally consistent (see the field comment on tpaScansCompleted). + snap.TPAScansCompleted = r.tpaScansCompleted + snap.TPAScansFailed = r.tpaScansFailed + snap.TPAScansWithFindings = r.tpaScansWithFindings + for k, v := range r.tpaFindings { + snap.TPAFindings[k] = v + } return snap } @@ -325,6 +450,10 @@ func (r *CounterRegistry) Reset() { r.restEndpoints = make(map[string]map[string]int64) r.errorCategories = make(map[ErrorCategory]int64) r.doctorChecks = make(map[string]*DoctorCounts) + r.tpaScansCompleted = 0 + r.tpaScansFailed = 0 + r.tpaScansWithFindings = 0 + r.tpaFindings = make(map[string]int64) } // bucketUpstream maps an upstream tool call count to its log bucket label. diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index 1964e7068..cbe74d751 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -69,7 +69,34 @@ import ( // All v7 fields are omitempty (zero-valued payloads stay shape-compatible // with v6), fixed-enum/boolean/non-negative-integer only (enforced by // ScanForPII), and ride the existing opt-out gate. -const SchemaVersion = 7 +// +// v8 (schema bump from 7): anonymous TPA / security-scanner stats. Additive +// only; v7-and-earlier consumers ignore both additions: +// - tpa_scanner (object, omitted entirely when every counter is zero — the +// same posture as diagnostics): scans_completed, scans_failed, +// scans_with_findings (non-negative integer counts over the reporting +// window, reset only after an accepted heartbeat) and findings, a sparse +// map from the FIXED severity enum (critical|high|medium|low|info) to a +// non-negative count. +// - feature_flags.deep_scan_enabled (bool): the opt-in deep-scan master +// switch (security.deep_scan.enabled), so scan volume can be read against +// the population that actually enabled the layer. +// +// The unit of every tpa_scanner counter is ONE NON-DEEP-SCAN (PASS 1) SCAN JOB: +// the Pass-2 deep supply-chain audit that deep scan auto-starts after Pass 1 is +// NOT counted (counting it would double the apparent scan volume of exactly the +// deep-scan cohort deep_scan_enabled exists to compare), dry-run jobs are NOT +// counted, and a job with several failing scanners is still one scan — +// scans_failed counts failed jobs, not failed scanners. The producer is +// scanCallbackAdapter.countsForTelemetry in internal/security/scanner. +// +// Anonymity properties: counts and fixed enum keys ONLY. Scanned server +// names, scanner ids, rule ids, finding titles, file paths, and error +// messages are never accepted by the counter API, and ScanForPII re-asserts +// the shape on the wire form (rule "v8_field_invalid"): tpa_scanner must be +// an object whose keys are whitelisted, whose scalar values are non-negative +// integers, and whose findings keys are members of the severity enum. +const SchemaVersion = 8 // HeartbeatPayload is the anonymous telemetry payload sent periodically. // Spec 042 expanded the payload with Tier 2 fields; v1 fields are preserved. @@ -248,6 +275,13 @@ type HeartbeatPayload struct { // all counters are zero (omitempty on the pointer). No PII: only stable // MCPX_* enum strings, non-negative int counts. Diagnostics *DiagnosticsCounters `json:"diagnostics,omitempty"` + + // Schema v8: anonymous TPA / security-scanner outcome counters. Omitted + // entirely when all counters are zero (omitempty on the pointer) — an + // install that never scans is shape-identical to a v7 payload. No PII: + // non-negative counts keyed by the fixed severity enum only; never a + // scanned server name, scanner id, rule id, or finding title. + TPAScanner *TPAScannerStats `json:"tpa_scanner,omitempty"` } // OnboardingSnapshot is the data the telemetry service needs to populate @@ -885,6 +919,9 @@ func (s *Service) buildHeartbeat() HeartbeatPayload { payload.RESTEndpointCalls = snap.RESTEndpointCalls payload.ErrorCategoryCounts = snap.ErrorCategoryCounts payload.DoctorChecks = snap.DoctorChecks + // Schema v8: security-scanner counters. nil (and therefore omitted) + // when the install never completed or failed a scan in the window. + payload.TPAScanner = snap.TPAScannerStats() } // Spec 046: onboarding funnel snapshot. Provider closes over connect.Service diff --git a/internal/telemetry/telemetry_test.go b/internal/telemetry/telemetry_test.go index d5d66253f..8abee01d6 100644 --- a/internal/telemetry/telemetry_test.go +++ b/internal/telemetry/telemetry_test.go @@ -306,16 +306,16 @@ func TestEnsureAnonymousID(t *testing.T) { // once the Spec 080 funnel/churn fields ship. This is a tripwire against // accidental downgrades. func TestSchemaVersionV7(t *testing.T) { - if SchemaVersion != 7 { - t.Fatalf("SchemaVersion = %d, want 7", SchemaVersion) + if SchemaVersion != 8 { + t.Fatalf("SchemaVersion = %d, want 8", SchemaVersion) } cfg := &config.Config{} svc := New(cfg, "", "v1.0.0", "personal", zap.NewNop()) svc.SetRuntimeStats(&mockRuntimeStats{}) payload := svc.BuildPayload() - if payload.SchemaVersion != 7 { - t.Errorf("payload.SchemaVersion = %d, want 7", payload.SchemaVersion) + if payload.SchemaVersion != 8 { + t.Errorf("payload.SchemaVersion = %d, want 8", payload.SchemaVersion) } } @@ -475,8 +475,8 @@ func TestAnonymousIDStable_V2ToV3(t *testing.T) { if p1.AnonymousID != p2.AnonymousID { t.Errorf("anonymous_id drifted between builds: %q vs %q", p1.AnonymousID, p2.AnonymousID) } - // SchemaVersion is 7 after the Spec 080 funnel/churn additions. - if p1.SchemaVersion != 7 { - t.Errorf("schema_version = %d, want 7 (Spec 080 additions)", p1.SchemaVersion) + // SchemaVersion is 8 after the schema-v8 TPA-scanner-stats additions. + if p1.SchemaVersion != 8 { + t.Errorf("schema_version = %d, want 8 (v8 tpa_scanner additions)", p1.SchemaVersion) } } diff --git a/internal/telemetry/tpa_scanner.go b/internal/telemetry/tpa_scanner.go new file mode 100644 index 000000000..4eef05ecf --- /dev/null +++ b/internal/telemetry/tpa_scanner.go @@ -0,0 +1,73 @@ +package telemetry + +// tpaSeverityKeys is the fixed severity enum emitted under +// tpa_scanner.findings. It mirrors the scanner's severity constants +// (internal/security/scanner/types.go) but is duplicated here deliberately: +// the telemetry package must not import the scanner package, and the enum is +// the anonymity contract — anything outside this list is dropped rather than +// transmitted. +var tpaSeverityKeys = []string{"critical", "high", "medium", "low", "info"} + +// tpaSeverityAllowList is the set form of tpaSeverityKeys. +var tpaSeverityAllowList = func() map[string]struct{} { + m := make(map[string]struct{}, len(tpaSeverityKeys)) + for _, k := range tpaSeverityKeys { + m[k] = struct{}{} + } + return m +}() + +// IsTPASeverity reports whether sev is a member of the fixed severity enum +// permitted in the heartbeat's tpa_scanner.findings map. +func IsTPASeverity(sev string) bool { + _, ok := tpaSeverityAllowList[sev] + return ok +} + +// TPAScannerStats is the schema-v8 security-scanner sub-object of the +// heartbeat payload. It answers "is the TPA / security scanner actually +// running in the fleet, does it fail, and does it find anything?" using +// counts alone. +// +// Unit of measure: ONE NON-DEEP-SCAN (PASS 1) SCAN JOB. The Pass-2 deep +// supply-chain audit and dry-run jobs are not counted, and a job with several +// failing scanners counts once — see internal/security/scanner +// (scanCallbackAdapter.countsForTelemetry), the only producer. +// +// Privacy contract (enforced by ScanForPII, rule "v8_field_invalid"): +// - every value is a non-negative integer count; +// - Findings keys are drawn ONLY from the fixed severity enum +// (critical/high/medium/low/info); +// - no server names, scanner ids, rule ids, finding titles, paths, or any +// other free text ever reaches this struct — the registry's Record* +// methods do not even accept them. +type TPAScannerStats struct { + // ScansCompleted is the number of terminal, successful scans in the + // reporting window (counters reset after each accepted heartbeat). + ScansCompleted int64 `json:"scans_completed"` + // ScansFailed is the number of terminal scan failures in the window. + ScansFailed int64 `json:"scans_failed"` + // ScansWithFindings is the subset of ScansCompleted that produced at + // least one finding of any severity. + ScansWithFindings int64 `json:"scans_with_findings"` + // Findings is the per-severity finding total across all completed scans + // in the window. Sparse: severities with a zero total are omitted. + Findings map[string]int64 `json:"findings,omitempty"` +} + +// isZero reports whether nothing at all was recorded, in which case the +// heartbeat omits the whole sub-object (same posture as DiagnosticsCounters). +func (t *TPAScannerStats) isZero() bool { + if t == nil { + return true + } + if t.ScansCompleted != 0 || t.ScansFailed != 0 || t.ScansWithFindings != 0 { + return false + } + for _, n := range t.Findings { + if n != 0 { + return false + } + } + return true +} diff --git a/internal/telemetry/tpa_scanner_test.go b/internal/telemetry/tpa_scanner_test.go new file mode 100644 index 000000000..e3591659c --- /dev/null +++ b/internal/telemetry/tpa_scanner_test.go @@ -0,0 +1,407 @@ +package telemetry + +import ( + "encoding/json" + "errors" + "strings" + "testing" + + "go.uber.org/zap" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" +) + +// TestRecordTPAScanCountsAndSeverities covers the happy path: completed and +// failed scans are counted separately, a scan with any finding also bumps +// scans_with_findings, and per-severity totals accumulate across scans. +func TestRecordTPAScanCountsAndSeverities(t *testing.T) { + r := NewCounterRegistry() + + r.RecordTPAScanCompleted(map[string]int{"high": 2, "low": 1}) + r.RecordTPAScanCompleted(map[string]int{"high": 1}) + r.RecordTPAScanCompleted(nil) // clean scan: no findings + r.RecordTPAScanCompleted(map[string]int{}) // clean scan: empty summary + r.RecordTPAScanCompleted(map[string]int{"info": 0}) // clean scan: zero count + r.RecordTPAScanFailed() + r.RecordTPAScanFailed() + + snap := r.Snapshot() + if snap.TPAScansCompleted != 5 { + t.Errorf("tpa_scans_completed = %d, want 5", snap.TPAScansCompleted) + } + if snap.TPAScansFailed != 2 { + t.Errorf("tpa_scans_failed = %d, want 2", snap.TPAScansFailed) + } + if snap.TPAScansWithFindings != 2 { + t.Errorf("tpa_scans_with_findings = %d, want 2", snap.TPAScansWithFindings) + } + if got := snap.TPAFindings["high"]; got != 3 { + t.Errorf("findings[high] = %d, want 3", got) + } + if got := snap.TPAFindings["low"]; got != 1 { + t.Errorf("findings[low] = %d, want 1", got) + } + // Every severity key is always present in the snapshot, even at zero. + for _, sev := range []string{"critical", "high", "medium", "low", "info"} { + if _, ok := snap.TPAFindings[sev]; !ok { + t.Errorf("snapshot findings missing fixed severity key %q", sev) + } + } + if len(snap.TPAFindings) != 5 { + t.Errorf("snapshot findings has %d keys, want exactly the 5 fixed severities", len(snap.TPAFindings)) + } +} + +// TestRecordTPAScanDropsUnknownSeverities is the privacy guard: any key that +// is not a member of the fixed severity enum (a rule id, a scanner id, a +// server name, a finding title) is silently dropped, and a scan whose findings +// are ALL unknown keys does not count as "with findings". +func TestRecordTPAScanDropsUnknownSeverities(t *testing.T) { + r := NewCounterRegistry() + + r.RecordTPAScanCompleted(map[string]int{ + "TPA-2026-0001": 4, + "github:create_issue": 1, + "/Users/algis/secret/path": 9, + "HIGH": 3, // case-sensitive: not the enum value + }) + + snap := r.Snapshot() + if snap.TPAScansCompleted != 1 { + t.Errorf("tpa_scans_completed = %d, want 1", snap.TPAScansCompleted) + } + if snap.TPAScansWithFindings != 0 { + t.Errorf("tpa_scans_with_findings = %d, want 0 (all keys were dropped)", snap.TPAScansWithFindings) + } + for k, v := range snap.TPAFindings { + if !IsTPASeverity(k) { + t.Errorf("snapshot leaked non-enum findings key %q", k) + } + if v != 0 { + t.Errorf("findings[%q] = %d, want 0", k, v) + } + } +} + +// TestRecordTPAScanIgnoresNegativeCounts asserts a negative severity count can +// never reach the payload (the anonymity contract is non-negative integers). +func TestRecordTPAScanIgnoresNegativeCounts(t *testing.T) { + r := NewCounterRegistry() + r.RecordTPAScanCompleted(map[string]int{"high": -5, "low": 2}) + + snap := r.Snapshot() + if got := snap.TPAFindings["high"]; got != 0 { + t.Errorf("findings[high] = %d, want 0 (negative dropped)", got) + } + if got := snap.TPAFindings["low"]; got != 2 { + t.Errorf("findings[low] = %d, want 2", got) + } + if snap.TPAScansWithFindings != 1 { + t.Errorf("tpa_scans_with_findings = %d, want 1", snap.TPAScansWithFindings) + } +} + +// TestRecordTPAScanOnNilRegistry pins the nil-safety contract of the *On +// wrappers — integration points may hold a nil registry when telemetry is not +// initialized. +func TestRecordTPAScanOnNilRegistry(t *testing.T) { + defer func() { + if rec := recover(); rec != nil { + t.Fatalf("nil-safe wrappers panicked: %v", rec) + } + }() + RecordTPAScanCompletedOn(nil, map[string]int{"high": 1}) + RecordTPAScanFailedOn(nil) + + // And they still work against a real registry. + r := NewCounterRegistry() + RecordTPAScanCompletedOn(r, map[string]int{"critical": 1}) + RecordTPAScanFailedOn(r) + snap := r.Snapshot() + if snap.TPAScansCompleted != 1 || snap.TPAScansFailed != 1 || snap.TPAFindings["critical"] != 1 { + t.Errorf("wrappers did not record: %+v", snap) + } +} + +// TestResetClearsTPACounters asserts the v8 counters participate in the +// post-heartbeat reset like every other windowed counter. +func TestResetClearsTPACounters(t *testing.T) { + r := NewCounterRegistry() + r.RecordTPAScanCompleted(map[string]int{"medium": 3}) + r.RecordTPAScanFailed() + + r.Reset() + + snap := r.Snapshot() + if snap.TPAScansCompleted != 0 || snap.TPAScansFailed != 0 || snap.TPAScansWithFindings != 0 { + t.Errorf("scan counters survived Reset: %+v", snap) + } + for sev, n := range snap.TPAFindings { + if n != 0 { + t.Errorf("findings[%q] = %d after Reset, want 0", sev, n) + } + } + if stats := snap.TPAScannerStats(); stats != nil { + t.Errorf("TPAScannerStats() = %+v after Reset, want nil (all-zero omission)", stats) + } +} + +// TestTPAScanSnapshotIsInternallyConsistent guards the atomics-vs-map tearing +// bug: the scalar counters and the findings map must move together under one +// lock, so a Snapshot concurrent with recording can never observe findings +// without the completed scan that produced them (nor scans_with_findings +// exceeding scans_completed). +func TestTPAScanSnapshotIsInternallyConsistent(t *testing.T) { + r := NewCounterRegistry() + + const scans = 500 + done := make(chan struct{}) + go func() { + defer close(done) + for i := 0; i < scans; i++ { + r.RecordTPAScanCompleted(map[string]int{"high": 1}) + r.RecordTPAScanFailed() + } + }() + + for i := 0; i < scans; i++ { + snap := r.Snapshot() + var totalFindings int64 + for _, n := range snap.TPAFindings { + totalFindings += n + } + if totalFindings > snap.TPAScansCompleted { + t.Fatalf("torn snapshot: %d findings but only %d completed scans", totalFindings, snap.TPAScansCompleted) + } + if snap.TPAScansWithFindings > snap.TPAScansCompleted { + t.Fatalf("torn snapshot: scans_with_findings=%d > scans_completed=%d", + snap.TPAScansWithFindings, snap.TPAScansCompleted) + } + } + <-done + + final := r.Snapshot() + if final.TPAScansCompleted != scans || final.TPAScansFailed != scans { + t.Errorf("final counts = completed %d / failed %d, want %d each", + final.TPAScansCompleted, final.TPAScansFailed, scans) + } + if final.TPAFindings["high"] != scans { + t.Errorf("findings[high] = %d, want %d", final.TPAFindings["high"], scans) + } +} + +// TestTPAScannerStatsOmittedWhenZero asserts the sub-object projection is nil +// (and therefore omitted from the payload) when nothing was recorded. +func TestTPAScannerStatsOmittedWhenZero(t *testing.T) { + if stats := NewCounterRegistry().Snapshot().TPAScannerStats(); stats != nil { + t.Fatalf("TPAScannerStats() = %+v on a fresh registry, want nil", stats) + } + // A failure alone is enough to make the sub-object non-nil. + r := NewCounterRegistry() + r.RecordTPAScanFailed() + stats := r.Snapshot().TPAScannerStats() + if stats == nil { + t.Fatal("TPAScannerStats() = nil after a failed scan, want non-nil") + } + if stats.Findings != nil { + t.Errorf("findings = %v, want nil (sparse: no severities recorded)", stats.Findings) + } +} + +// newTPAPayloadTestService builds a telemetry service with a deterministic +// config, mirroring newFunnelPayloadTestService but with the deep-scan switch +// controllable. +func newTPAPayloadTestService(t *testing.T, deepScan bool) *Service { + t.Helper() + t.Setenv("DO_NOT_TRACK", "") + t.Setenv("CI", "") + t.Setenv("MCPPROXY_TELEMETRY", "") + + cfg := &config.Config{ + EnableSocket: true, + Features: &config.FeatureFlags{EnableWebUI: true}, + Telemetry: &config.TelemetryConfig{ + AnonymousID: "550e8400-e29b-41d4-a716-446655440000", + AnonymousIDCreatedAt: "2026-04-10T12:00:00Z", + }, + } + if deepScan { + cfg.Security = &config.SecurityConfig{ + DeepScan: &config.DeepScanConfig{Enabled: true}, + } + } + return New(cfg, "", "v1.2.3", "personal", zap.NewNop()) +} + +// TestPayloadV8_TPAScannerIncludedAndAnonymous is the v8 contract test: a +// payload carrying scanner activity reaches the wire with counts + fixed +// severity keys only, and passes the anonymity scanner. +func TestPayloadV8_TPAScannerIncludedAndAnonymous(t *testing.T) { + svc := newTPAPayloadTestService(t, true) + reg := svc.Registry() + reg.RecordTPAScanCompleted(map[string]int{"critical": 1, "high": 2}) + reg.RecordTPAScanCompleted(nil) + reg.RecordTPAScanFailed() + + payload := svc.BuildPayload() + if payload.TPAScanner == nil { + t.Fatal("payload.tpa_scanner = nil, want the v8 sub-object") + } + if payload.TPAScanner.ScansCompleted != 2 { + t.Errorf("scans_completed = %d, want 2", payload.TPAScanner.ScansCompleted) + } + if payload.TPAScanner.ScansFailed != 1 { + t.Errorf("scans_failed = %d, want 1", payload.TPAScanner.ScansFailed) + } + if payload.TPAScanner.ScansWithFindings != 1 { + t.Errorf("scans_with_findings = %d, want 1", payload.TPAScanner.ScansWithFindings) + } + + data, err := json.Marshal(payload) + if err != nil { + t.Fatalf("marshal: %v", err) + } + js := string(data) + + for _, required := range []string{ + `"schema_version":8`, + `"tpa_scanner":`, + `"scans_completed":2`, + `"scans_failed":1`, + `"scans_with_findings":1`, + `"critical":1`, + `"high":2`, + `"deep_scan_enabled":true`, + } { + if !strings.Contains(js, required) { + t.Errorf("expected v8 payload to contain %s, missing from:\n%s", required, js) + } + } + // Sparse findings: severities that were never seen do not appear. + if strings.Contains(js, `"medium"`) { + t.Errorf("zero-valued severity leaked into findings:\n%s", js) + } + + prev := BlockedValues + BlockedValues = nil + defer func() { BlockedValues = prev }() + if scanErr := ScanForPII(data); scanErr != nil { + t.Fatalf("v8 payload with tpa_scanner must pass ScanForPII, got: %v\npayload:\n%s", scanErr, js) + } +} + +// TestPayloadV8_TPAScannerOmittedWhenNoScans asserts the additive-only +// contract: an install that never scanned emits a payload shape-identical to +// v7 (no tpa_scanner key at all). +func TestPayloadV8_TPAScannerOmittedWhenNoScans(t *testing.T) { + svc := newTPAPayloadTestService(t, false) + + payload := svc.BuildPayload() + if payload.TPAScanner != nil { + t.Errorf("payload.tpa_scanner = %+v, want nil when no scan ran", payload.TPAScanner) + } + + data, err := json.Marshal(payload) + if err != nil { + t.Fatalf("marshal: %v", err) + } + js := string(data) + if strings.Contains(js, `"tpa_scanner"`) { + t.Errorf("tpa_scanner must be omitted on a zero-valued payload, got:\n%s", js) + } + if !strings.Contains(js, `"deep_scan_enabled":false`) { + t.Errorf("expected deep_scan_enabled:false, got:\n%s", js) + } +} + +// TestBuildFeatureFlagSnapshot_DeepScan covers the v8 feature flag across the +// nil-config, nil-security, disabled, and enabled cases. +func TestBuildFeatureFlagSnapshot_DeepScan(t *testing.T) { + if snap := BuildFeatureFlagSnapshot(nil); snap.DeepScanEnabled { + t.Error("deep_scan_enabled = true for a nil config, want false") + } + if snap := BuildFeatureFlagSnapshot(&config.Config{}); snap.DeepScanEnabled { + t.Error("deep_scan_enabled = true with no security block, want false") + } + cfgNilDeep := &config.Config{Security: &config.SecurityConfig{}} + if snap := BuildFeatureFlagSnapshot(cfgNilDeep); snap.DeepScanEnabled { + t.Error("deep_scan_enabled = true with a nil deep_scan block, want false") + } + cfgOff := &config.Config{Security: &config.SecurityConfig{DeepScan: &config.DeepScanConfig{Enabled: false}}} + if snap := BuildFeatureFlagSnapshot(cfgOff); snap.DeepScanEnabled { + t.Error("deep_scan_enabled = true while deep_scan.enabled=false") + } + cfgOn := &config.Config{Security: &config.SecurityConfig{DeepScan: &config.DeepScanConfig{Enabled: true}}} + if snap := BuildFeatureFlagSnapshot(cfgOn); !snap.DeepScanEnabled { + t.Error("deep_scan_enabled = false while deep_scan.enabled=true") + } +} + +// TestScanForPII_TPAScannerShapeViolations pins the wire-form backstop: even +// if producer-side filtering regressed, a tpa_scanner sub-object carrying a +// non-enum key, a negative count, a string count, or an unknown field is +// rejected before transmit. +func TestScanForPII_TPAScannerShapeViolations(t *testing.T) { + prev := BlockedValues + BlockedValues = nil + defer func() { BlockedValues = prev }() + + clean := []string{ + `{"anonymous_id":"abc","schema_version":8}`, + `{"anonymous_id":"abc","schema_version":8,"tpa_scanner":{"scans_completed":0,"scans_failed":0,"scans_with_findings":0}}`, + `{"anonymous_id":"abc","schema_version":8,"tpa_scanner":{"scans_completed":3,"scans_failed":1,"scans_with_findings":2,` + + `"findings":{"critical":1,"high":2,"medium":3,"low":4,"info":5}}}`, + } + for _, js := range clean { + if err := ScanForPII([]byte(js)); err != nil { + t.Errorf("clean payload rejected: %v\n%s", err, js) + } + } + + dirty := []struct { + name string + payload string + }{ + {"non-enum findings key (rule id)", + `{"tpa_scanner":{"findings":{"TPA-2026-0001":2}}}`}, + {"non-enum findings key (server name)", + `{"tpa_scanner":{"findings":{"my-private-server":1}}}`}, + {"negative scalar", + `{"tpa_scanner":{"scans_completed":-1}}`}, + {"negative finding count", + `{"tpa_scanner":{"findings":{"high":-2}}}`}, + {"string scalar", + `{"tpa_scanner":{"scans_failed":"3"}}`}, + {"string finding count", + `{"tpa_scanner":{"findings":{"high":"many"}}}`}, + {"unknown sub-key", + `{"tpa_scanner":{"scans_completed":1,"server_name":"github"}}`}, + {"not an object", + `{"tpa_scanner":"github"}`}, + {"findings not an object", + `{"tpa_scanner":{"findings":["high"]}}`}, + } + for _, tc := range dirty { + err := ScanForPII([]byte(tc.payload)) + if err == nil { + t.Errorf("%s: expected an anonymity violation, got nil for %s", tc.name, tc.payload) + continue + } + var v *AnonymityViolation + if !errors.As(err, &v) { + t.Errorf("%s: expected *AnonymityViolation, got %T", tc.name, err) + continue + } + if v.Rule != "v8_field_invalid" { + t.Errorf("%s: rule = %q, want v8_field_invalid", tc.name, v.Rule) + } + // The violation must never echo the offending (possibly identifying) + // map key back into logs. + for _, secret := range []string{"TPA-2026-0001", "my-private-server", "github"} { + if strings.Contains(v.Pattern, secret) || strings.Contains(v.Reason, secret) { + t.Errorf("%s: violation echoed %q back: pattern=%q reason=%q", + tc.name, secret, v.Pattern, v.Reason) + } + } + } +} From e9afc977998d545929aaeed044a38ba824b7c302 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 5 Aug 2026 13:19:50 +0300 Subject: [PATCH 2/4] fix(tray): align the status-bar menu and open Activity natively MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four visual defects and one routing defect in the tray menu: - Histogram now spans the full menu width: the NSHostingView gets a flexible-width autoresizing mask (SwiftUI maxWidth .infinity, 272pt floor) instead of a fixed frame that left a dead band whenever a text row was wider. The cached chart item resets its width when reused so a once-wide menu cannot ratchet permanently. - 'Recent' and 'Clients' are real NSMenuItem.sectionHeaders on macOS 14+ (disabled-row fallback below), not full-size disabled rows. - Successful Recent rows carry an invisible 16x16 placeholder image so every title in the section shares one leading edge while failures stay the only visible marks (FR-010); all glance glyphs normalized to 16pt. - Start/Stop/Disconnect Core items lose their oversized 18x18 icons — the bottom command block now sits on one leading edge like its neighbours. - 'Open Activity…' and Recent rows open the native main window at the Activity section (showMainWindow(tab:) now honours its previously-dead parameter; fresh windows seed via MainWindow(initialTab:), live windows via a new .switchToSidebarTab notification; a miniaturized window is deminiaturized instead of duplicated). The Web-UI deep-link path (GlanceLinks) is deleted; Web UI stays reachable via 'Open Web UI'. Verified live via mcpproxy-ui-test against an isolated core with seeded success/error/blocked activity. 788 tests pass; new pins cover section headers, chart autoresizing, the notification decode seam and the glance routing destination. --- .../macos/MCPProxy/MCPProxy/MCPProxyApp.swift | 58 +++++++++------ .../Menu/Glance/ActivityHistogramView.swift | 25 +++++-- .../MCPProxy/Menu/Glance/GlanceLinks.swift | 30 -------- .../MCPProxy/Menu/Glance/GlanceSection.swift | 73 ++++++++++++++++--- .../MCPProxy/MCPProxy/Views/MainWindow.swift | 25 ++++++- .../ActivityHistogramTests.swift | 3 + .../MCPProxyTests/GlanceLinksTests.swift | 43 ----------- .../MCPProxyTests/GlanceRowRoutingTests.swift | 11 +-- .../MCPProxyTests/GlanceSectionTests.swift | 38 ++++++++-- .../MainWindowRoutingTests.swift | 43 +++++++++++ 10 files changed, 219 insertions(+), 130 deletions(-) delete mode 100644 native/macos/MCPProxy/MCPProxy/Menu/Glance/GlanceLinks.swift delete mode 100644 native/macos/MCPProxy/MCPProxyTests/GlanceLinksTests.swift create mode 100644 native/macos/MCPProxy/MCPProxyTests/MainWindowRoutingTests.swift diff --git a/native/macos/MCPProxy/MCPProxy/MCPProxyApp.swift b/native/macos/MCPProxy/MCPProxy/MCPProxyApp.swift index 709076739..ecf1eeba3 100644 --- a/native/macos/MCPProxy/MCPProxy/MCPProxyApp.swift +++ b/native/macos/MCPProxy/MCPProxy/MCPProxyApp.swift @@ -73,8 +73,8 @@ final class AppController: NSObject, NSApplicationDelegate, NSWindowDelegate, NS /// Tray Glance: builds the activity / clients / histogram rows, and keeps /// references to them so a refresh landing while the menu is on screen can /// rewrite them in place instead of restructuring the menu. Rows call back - /// into this delegate (see `openActivityForSession`) so Web UI key handling - /// stays in one place — the section is handed only AppState, which has no key. + /// into this delegate (see `openActivityForSession`), which opens the + /// native main window at the Activity section. /// /// `@MainActor` because `GlanceSection` is an isolated type and this class is /// not, so constructing it from a plain stored-property initializer would not @@ -379,11 +379,20 @@ final class AppController: NSObject, NSApplicationDelegate, NSWindowDelegate, NS /// /// - Parameter tab: Optional sidebar item to select when the window opens. func showMainWindow(tab: SidebarItem? = nil) { - if let window = mainWindow, window.isVisible { + // isVisible is false for a miniaturized window — falling through to + // window creation there would leave the original in the Dock and + // spawn a duplicate, with both subscribed to the tab notifications. + if let window = mainWindow, window.isVisible || window.isMiniaturized { + if window.isMiniaturized { window.deminiaturize(nil) } NSApp.setActivationPolicy(.regular) setupMainMenu() // Reapply our menu when becoming regular app window.makeKeyAndOrderFront(nil) NSApp.activate(ignoringOtherApps: true) + // The window is already live, so its `onReceive` observers are + // subscribed — a notification is the reliable path to switch tabs. + if let tab { + NotificationCenter.default.post(name: .switchToSidebarTab, object: tab.rawValue) + } return } @@ -399,7 +408,11 @@ final class AppController: NSObject, NSApplicationDelegate, NSWindowDelegate, NS // MainWindow reads apiClient from appState, so we create it once. // When appState.apiClient is set by CoreProcessManager, all views // automatically re-render — no need to replace the NSHostingView. - let contentView = MainWindow(appState: appState) + // + // A fresh window gets its tab as initial state rather than a + // notification: the `onReceive` observers only subscribe once the view + // appears, so a notification posted now would be dropped on the floor. + let contentView = MainWindow(appState: appState, initialTab: tab ?? .dashboard) let hostingView = NSHostingView(rootView: contentView) let window = NSWindow( @@ -1125,11 +1138,14 @@ final class AppController: NSObject, NSApplicationDelegate, NSWindowDelegate, NS menu.addItem(.separator()) // Stop / Start + // No icons on the lifecycle commands: every other command in the + // bottom half of the menu (Add Server…, Settings…, Run at Startup, + // Quit) is a bare title, and a per-item image indents only its own + // title — an icon here put "Stop MCPProxy Core" on a different + // leading edge from its neighbours. if appState.isStopped { let start = NSMenuItem(title: "Start MCPProxy Core", action: #selector(startCoreAction), keyEquivalent: "") start.target = self - start.image = NSImage(systemSymbolName: "play.circle.fill", accessibilityDescription: "start") - start.image?.size = NSSize(width: 18, height: 18) menu.addItem(start) } else if appState.coreState == .connected || appState.coreState.isOperational { // A core we only attached to cannot be stopped by us — we hold no PID @@ -1138,9 +1154,6 @@ final class AppController: NSObject, NSApplicationDelegate, NSWindowDelegate, NS let ownership = appState.ownership let stop = NSMenuItem(title: ownership.stopActionTitle, action: #selector(stopCore), keyEquivalent: "") stop.target = self - let symbol = ownership.shouldTerminateOnShutdown ? "stop.circle.fill" : "eject.circle.fill" - stop.image = NSImage(systemSymbolName: symbol, accessibilityDescription: "stop") - stop.image?.size = NSSize(width: 18, height: 18) if !ownership.shouldTerminateOnShutdown { stop.toolTip = "This core was started outside MCPProxy. Disconnecting leaves it running." } @@ -1328,22 +1341,20 @@ final class AppController: NSObject, NSApplicationDelegate, NSWindowDelegate, NS /// Open the Web UI activity log filtered by a glance row's session. /// - /// Reuses `openWebUI()`'s key path: `webUIBaseURL` is scheme/host/port only - /// and a first-time browser session needs the API key appended, which only - /// the core manager holds. A row with no session id (an empty-state row, or - /// a record the core never attributed) opens the unfiltered log. + /// Opens the app's own window at the Activity section — the activity log + /// has a native home, and a tray click should not context-switch the user + /// into a browser (the Web UI stays reachable via "Open Web UI"). The + /// row's session id (representedObject) stays on the item because the + /// glance in-place update reads it as row identity; the native log + /// currently opens unfiltered. @objc private func openActivityForSession(_ sender: NSMenuItem) { - let sessionID = sender.representedObject as? String - Task { - let apiKey = await coreManager?.currentAPIKey ?? "" - let baseURL = await MainActor.run { appState.webUIBaseURL } - let urlString = activityURLString(baseURL: baseURL, apiKey: apiKey, sessionID: sessionID) - if let url = URL(string: urlString) { - NSWorkspace.shared.open(url) - } - } + showMainWindow(tab: Self.glanceActivityDestination) } + /// Where a glance row click lands. A constant so tests can pin the + /// destination without instantiating the app delegate. + static let glanceActivityDestination: SidebarItem = .activity + @objc private func openConfigFile() { NSWorkspace.shared.open(InstancePaths.configFileURL) } @@ -1437,6 +1448,9 @@ extension Notification.Name { static let switchToServers = Notification.Name("MCPProxy.switchToServers") /// Posted by tray menu to open the detail view for a specific server (object = server name string). static let showServerDetail = Notification.Name("MCPProxy.showServerDetail") + /// Posted by `showMainWindow(tab:)` to select a sidebar section in an + /// already-open main window (object = SidebarItem raw value string). + static let switchToSidebarTab = Notification.Name("MCPProxy.switchToSidebarTab") } @main diff --git a/native/macos/MCPProxy/MCPProxy/Menu/Glance/ActivityHistogramView.swift b/native/macos/MCPProxy/MCPProxy/Menu/Glance/ActivityHistogramView.swift index 689132a36..02cc0665b 100644 --- a/native/macos/MCPProxy/MCPProxy/Menu/Glance/ActivityHistogramView.swift +++ b/native/macos/MCPProxy/MCPProxy/Menu/Glance/ActivityHistogramView.swift @@ -197,7 +197,12 @@ struct ActivityHistogramView: View { // the chart is a shape to recognise, not a plot to read values off — // relative bar heights survive 60 pt of plot, and a menu is the wrong // place for more (the Web UI has the full-size version). - .frame(width: 248, height: 84) + // + // Width is elastic: 248 pt is the floor that keeps the axis readable, + // but when another row makes the menu wider the chart must follow — + // a fixed-width chart in a wider menu reads as a band of dead space + // on the right (the hosting view stretches via its autoresizing mask). + .frame(minWidth: 248, maxWidth: .infinity, minHeight: 84, maxHeight: 84) .padding(.horizontal, 12) .padding(.vertical, 6) // One label for the whole chart: VoiceOver reading 48 unlabelled bar @@ -209,11 +214,15 @@ struct ActivityHistogramView: View { extension ActivityHistogram { - /// Size of the hosted chart item, in points. Menu items do not auto-size a - /// hosting view, so the frame is explicit — and it must match the view's - /// own size, or the row grows a band of dead space. 248 + 2*12 = 272 wide, - /// 84 + 2*6 = 96 tall; measured `NSHostingView.fittingSize` agrees, and - /// `testRealChartItemIsSizedAndLabelled` keeps the two in step. + /// Minimum size of the hosted chart item, in points. Menu items do not + /// auto-size a hosting view, so the frame is explicit — and it must match + /// the view's own minimum size, or the row grows a band of dead space. + /// 248 + 2*12 = 272 wide, 84 + 2*6 = 96 tall; measured + /// `NSHostingView.fittingSize` agrees, and + /// `testRealChartItemIsSizedAndLabelled` keeps the two in step. Width is a + /// floor, not a fix: the host's flexible-width autoresizing mask lets + /// AppKit stretch the row to the menu's final width, and the SwiftUI chart + /// (maxWidth: .infinity) fills whatever it is given. static let chartItemSize = NSSize(width: 272, height: 96) /// The glance's single custom item: an `NSHostingView` wrapping the chart. @@ -230,6 +239,10 @@ extension ActivityHistogram { rootView: ActivityHistogramView(bars: bars, accessibilitySummary: summary) ) host.frame = NSRect(origin: .zero, size: chartItemSize) + // The menu stretches an item view to its final width only when the + // view opts in — without this the chart stays 272 pt wide and any + // wider row leaves a dead band on the chart's right edge. + host.autoresizingMask = [.width] host.setAccessibilityLabel(summary) item.view = host return item diff --git a/native/macos/MCPProxy/MCPProxy/Menu/Glance/GlanceLinks.swift b/native/macos/MCPProxy/MCPProxy/Menu/Glance/GlanceLinks.swift deleted file mode 100644 index 1fdd2c123..000000000 --- a/native/macos/MCPProxy/MCPProxy/Menu/Glance/GlanceLinks.swift +++ /dev/null @@ -1,30 +0,0 @@ -// GlanceLinks.swift -// MCPProxy -// -// Web UI deep links opened from glance rows. - -import Foundation - -/// Build the Web UI activity-log URL, optionally filtered by session. -/// -/// `?session=` is the query parameter the Activity view reads on mount -/// (frontend/src/views/Activity.vue:1334, `route.query.session`), and the Web -/// UI router is history-based (createWebHistory over `base: '/ui/'`), so -/// `/ui/activity` is a real path rather than a fragment. `apikey` travels as a -/// query parameter because a browser cannot send the `X-API-Key` header — -/// `/ui/` is the one surface that accepts it, and the Web UI strips only -/// `apikey` from the address bar on load (services/api.ts:69-80), keeping -/// `session`. -func activityURLString(baseURL: String, apiKey: String, sessionID: String?) -> String { - let path = baseURL + "/ui/activity" - var query: [URLQueryItem] = [] - if let sessionID, !sessionID.isEmpty { - query.append(URLQueryItem(name: "session", value: sessionID)) - } - if !apiKey.isEmpty { - query.append(URLQueryItem(name: "apikey", value: apiKey)) - } - guard var components = URLComponents(string: path) else { return path } - components.queryItems = query.isEmpty ? nil : query - return components.string ?? path -} diff --git a/native/macos/MCPProxy/MCPProxy/Menu/Glance/GlanceSection.swift b/native/macos/MCPProxy/MCPProxy/Menu/Glance/GlanceSection.swift index ea88d6050..b9dc7435c 100644 --- a/native/macos/MCPProxy/MCPProxy/Menu/Glance/GlanceSection.swift +++ b/native/macos/MCPProxy/MCPProxy/Menu/Glance/GlanceSection.swift @@ -13,11 +13,11 @@ // opens (it used to hide in an "Activity (24h)" submenu, which made the // overview the only row that required a second navigation step). // -// This component never builds a Web UI URL. It is handed only AppState, whose -// webUIBaseURL is scheme/host/port by design, while the API key lives on the -// core manager. Rows therefore carry a target/action pair plus a -// representedObject holding the record's session id, and the app delegate opens -// the authenticated URL through the same path as every other menu action. +// Clicking a row (or "Open Activity…") opens the app's native window at the +// Activity section — never a browser. Rows carry a target/action pair into the +// app delegate, plus a representedObject holding the record's session id; the +// id is what the in-place update uses to tell "same run, later clock" from +// "this row now shows a different record" (see `apply(_:to:now:)`). // // @MainActor as a type: every member here either builds live NSMenuItems or // reads AppState, so main-thread-only is the truth about this component, and @@ -192,7 +192,7 @@ final class GlanceSection { // the full log — a header over an empty list explains nothing. let runs = Self.recentRuns(for: state, now: now) if !runs.isEmpty { - items.append(disabledItem(titled: "Recent")) + items.append(sectionHeaderItem(titled: "Recent")) for run in runs { var row = ActivityRow(item: actionableItem()) apply(run, to: &row, now: now) @@ -203,12 +203,16 @@ final class GlanceSection { let openActivity = actionableItem() openActivity.title = "Open Activity…" - openActivity.image = NSImage(systemSymbolName: "list.bullet.rectangle", - accessibilityDescription: "activity log") + let activityIcon = NSImage(systemSymbolName: "list.bullet.rectangle", + accessibilityDescription: "activity log") + // Same slot width as the row glyphs above, so this title shares their + // leading edge instead of sitting a few points off. + activityIcon?.size = Self.rowImageSize + openActivity.image = activityIcon items.append(openActivity) items.append(.separator()) - items.append(disabledItem(titled: "Clients")) + items.append(sectionHeaderItem(titled: "Clients")) let presence = Self.clientList(for: state, now: now) if presence.rows.isEmpty { items.append(disabledItem(titled: Self.noClientsTitle)) @@ -567,7 +571,8 @@ final class GlanceSection { } /// The row icon: an SF Symbol whose shape carries the outcome, tinted to - /// carry it a second time — or nothing at all, when the call succeeded. + /// carry it a second time — or a clear placeholder, when the call + /// succeeded. /// /// Success is deliberately unmarked (FR-010). In a real 6-week export 1,480 /// of 1,564 outcome-bearing events succeeded, so a green tick appeared on @@ -575,15 +580,32 @@ final class GlanceSection { /// errors and 52 blocks among identical-looking rows. A mark now means /// "look at this". /// + /// Unmarked is not the same as slotless: AppKit reserves the image column + /// per item, so a success row with a nil image would start its title a + /// full icon-width left of a failed sibling's — rows in one section at two + /// x-origins. The placeholder is invisible but keeps every Recent title on + /// the same leading edge. + /// /// The image must be non-template — AppKit recolours a template menu image /// to the menu's own text colour, which would silently discard the tint. private static func statusImage(forStatus status: String) -> NSImage? { - guard status != "success" else { return nil } + guard status != "success" else { return clearRowImage } return symbolImage(named: GlanceFormatting.statusSymbolName(forStatus: status), tint: statusTint(forStatus: status), description: outcomeDescription(forStatus: status)) } + /// One point size for every image in the glance rows, so titles align + /// regardless of which glyph (or none) a row carries. + static let rowImageSize = NSSize(width: 16, height: 16) + + /// A fully transparent image the size of a row glyph. VoiceOver ignores it + /// (no accessibility description), eyes ignore it — only layout sees it. + /// Internal so tests can assert "visibly unmarked" by identity. + static let clearRowImage: NSImage = { + NSImage(size: rowImageSize, flipped: false) { _ in true } + }() + private static func symbolImage(named name: String, tint: NSColor, description: String) -> NSImage? { guard let base = NSImage(systemSymbolName: name, accessibilityDescription: description) else { return nil @@ -591,6 +613,7 @@ final class GlanceSection { let tinted = base.withSymbolConfiguration(NSImage.SymbolConfiguration(paletteColors: [tint])) ?? base tinted.isTemplate = false tinted.accessibilityDescription = description + tinted.size = rowImageSize return tinted } @@ -732,6 +755,15 @@ final class GlanceSection { builtHistogramKind = .chart if let cached = histogramRow, builtHistogramBars == bars, builtHistogramTimeZoneID == TimeZone.current.identifier { + // Undo the width ratchet: the last display stretched the + // hosted view to that menu's final width, and a view-backed + // item's frame is itself a width floor — returning it as-is + // would keep the menu as wide as its widest-ever row. Reset + // to the minimum and let the next display stretch it again. + if let view = cached.view, view.frame.width != ActivityHistogram.chartItemSize.width { + view.setFrameSize(NSSize(width: ActivityHistogram.chartItemSize.width, + height: view.frame.height)) + } return cached } let item = histogramChartItemFactory(bars) @@ -769,7 +801,12 @@ final class GlanceSection { switch (builtKind == .chart, newKind == .chart) { case (true, true): guard case .loaded(let bars) = histogramState, builtHistogramBars != bars else { return } - item.view = histogramChartItemFactory(bars).view + let replacement = histogramChartItemFactory(bars).view + // The menu already stretched the old view to the menu's final + // width; a factory-fresh view still has the minimum frame, and + // swapping it in mid-open would visibly shrink the chart. + if let current = item.view, let replacement { replacement.frame = current.frame } + item.view = replacement builtHistogramBars = bars builtHistogramTimeZoneID = TimeZone.current.identifier case (false, false): @@ -865,6 +902,18 @@ final class GlanceSection { return item } + /// A section header ("Recent", "Clients") rendered the way the system + /// renders its own menu sections — small grey caps with the standard + /// header margin — instead of a full-size disabled row masquerading as + /// one. Pre-macOS 14 there is no header style, so the disabled row is the + /// documented degradation. + private func sectionHeaderItem(titled title: String) -> NSMenuItem { + if #available(macOS 14.0, *) { + return NSMenuItem.sectionHeader(title: title) + } + return disabledItem(titled: title) + } + private func actionableItem() -> NSMenuItem { let item = NSMenuItem(title: "", action: clickAction, keyEquivalent: "") item.target = clickTarget diff --git a/native/macos/MCPProxy/MCPProxy/Views/MainWindow.swift b/native/macos/MCPProxy/MCPProxy/Views/MainWindow.swift index 169a8fcb9..bf0520c3c 100644 --- a/native/macos/MCPProxy/MCPProxy/Views/MainWindow.swift +++ b/native/macos/MCPProxy/MCPProxy/Views/MainWindow.swift @@ -26,7 +26,16 @@ enum SidebarItem: String, CaseIterable, Identifiable { struct MainWindow: View { @ObservedObject var appState: AppState - @State private var selectedItem: SidebarItem? = .dashboard + @State private var selectedItem: SidebarItem? + + /// `initialTab` seeds the sidebar selection for a window created to land + /// on a specific section (tray "Open Activity…" → Activity). Once the + /// window exists, later switches arrive as `.switchToSidebarTab` + /// notifications instead — state is only readable at creation time. + init(appState: AppState, initialTab: SidebarItem = .dashboard) { + self.appState = appState + _selectedItem = State(initialValue: initialTab) + } var body: some View { NavigationSplitView { @@ -82,6 +91,20 @@ struct MainWindow: View { .onReceive(NotificationCenter.default.publisher(for: .switchToServers)) { _ in selectedItem = .servers } + .onReceive(NotificationCenter.default.publisher(for: .switchToSidebarTab)) { note in + guard let item = MainWindow.sidebarItem(from: note) else { return } + selectedItem = item + } + } + + /// Decode a `.switchToSidebarTab` notification's payload. The wire form is + /// the SidebarItem raw value as a String (posted by + /// `AppController.showMainWindow(tab:)`); anything else — including a + /// SidebarItem posted as the object itself — is deliberately dropped + /// rather than crashing a notification handler. + static func sidebarItem(from note: Notification) -> SidebarItem? { + guard let raw = note.object as? String else { return nil } + return SidebarItem(rawValue: raw) } /// Hidden ⌘1…⌘5 shortcuts to jump straight to each sidebar section. Keeps diff --git a/native/macos/MCPProxy/MCPProxyTests/ActivityHistogramTests.swift b/native/macos/MCPProxy/MCPProxyTests/ActivityHistogramTests.swift index 9a3b777a9..635d9570d 100644 --- a/native/macos/MCPProxy/MCPProxyTests/ActivityHistogramTests.swift +++ b/native/macos/MCPProxy/MCPProxyTests/ActivityHistogramTests.swift @@ -511,6 +511,9 @@ final class GlanceInlineHistogramTests: XCTestCase { XCTAssertEqual(item.view?.frame.size, ActivityHistogram.chartItemSize) XCTAssertEqual(item.view?.fittingSize, ActivityHistogram.chartItemSize, "the hosting view must fit its frame exactly, or the row grows dead space") + XCTAssertEqual(item.view?.autoresizingMask, [.width], + "without flexible width the menu never stretches the chart, " + + "and any wider row leaves a dead band on the chart's right edge") XCTAssertEqual(item.view?.accessibilityLabel(), "Activity over the last 24 hours: no tool calls.") XCTAssertFalse(item.isEnabled) diff --git a/native/macos/MCPProxy/MCPProxyTests/GlanceLinksTests.swift b/native/macos/MCPProxy/MCPProxyTests/GlanceLinksTests.swift deleted file mode 100644 index 6bf1dd44c..000000000 --- a/native/macos/MCPProxy/MCPProxyTests/GlanceLinksTests.swift +++ /dev/null @@ -1,43 +0,0 @@ -import XCTest -@testable import MCPProxy - -final class GlanceLinksTests: XCTestCase { - - func testSessionAndKeyAreBothAppended() { - XCTAssertEqual( - activityURLString(baseURL: "http://127.0.0.1:8080", apiKey: "k1", sessionID: "sess-42"), - "http://127.0.0.1:8080/ui/activity?session=sess-42&apikey=k1" - ) - } - - func testMissingKeyOmitsTheParameter() { - XCTAssertEqual( - activityURLString(baseURL: "http://127.0.0.1:8080", apiKey: "", sessionID: "sess-42"), - "http://127.0.0.1:8080/ui/activity?session=sess-42" - ) - } - - func testMissingSessionOpensTheUnfilteredLog() { - XCTAssertEqual( - activityURLString(baseURL: "http://127.0.0.1:8080", apiKey: "k1", sessionID: nil), - "http://127.0.0.1:8080/ui/activity?apikey=k1" - ) - XCTAssertEqual( - activityURLString(baseURL: "http://127.0.0.1:8080", apiKey: "", sessionID: ""), - "http://127.0.0.1:8080/ui/activity" - ) - } - - func testSessionIDIsPercentEncoded() { - let url = activityURLString(baseURL: "http://127.0.0.1:8080", apiKey: "", sessionID: "a b&c") - XCTAssertEqual(url, "http://127.0.0.1:8080/ui/activity?session=a%20b%26c") - XCTAssertNotNil(URL(string: url)) - } - - func testNonDefaultPortIsPreserved() { - XCTAssertEqual( - activityURLString(baseURL: "http://127.0.0.1:18080", apiKey: "k", sessionID: "s"), - "http://127.0.0.1:18080/ui/activity?session=s&apikey=k" - ) - } -} diff --git a/native/macos/MCPProxy/MCPProxyTests/GlanceRowRoutingTests.swift b/native/macos/MCPProxy/MCPProxyTests/GlanceRowRoutingTests.swift index 57a533823..c378c46bf 100644 --- a/native/macos/MCPProxy/MCPProxyTests/GlanceRowRoutingTests.swift +++ b/native/macos/MCPProxy/MCPProxyTests/GlanceRowRoutingTests.swift @@ -48,7 +48,7 @@ final class GlanceRowRoutingTests: XCTestCase { /// Two producers make `representedObject` nil: a record the core never /// attributed to a session, and the "Open Activity…" row itself. Both mean - /// the same thing downstream — open the unfiltered log. + /// the same thing downstream — open the log with no session context. func testNilRepresentedObjectOpensTheUnfilteredLog() throws { let state = GlanceFixtures.connectedState() state.glanceActivity = [ @@ -66,14 +66,5 @@ final class GlanceRowRoutingTests: XCTestCase { "a record with no session_id must not carry a stale id") XCTAssertNil(openActivityRow.representedObject, "the Open Activity… row is deliberately unfiltered") - - for row in [unattributedRow, openActivityRow] { - XCTAssertEqual( - activityURLString(baseURL: "http://127.0.0.1:8080", - apiKey: "", - sessionID: row.representedObject as? String), - "http://127.0.0.1:8080/ui/activity" - ) - } } } diff --git a/native/macos/MCPProxy/MCPProxyTests/GlanceSectionTests.swift b/native/macos/MCPProxy/MCPProxyTests/GlanceSectionTests.swift index c954d193e..5b65e31e5 100644 --- a/native/macos/MCPProxy/MCPProxyTests/GlanceSectionTests.swift +++ b/native/macos/MCPProxy/MCPProxyTests/GlanceSectionTests.swift @@ -281,7 +281,8 @@ final class GlanceSectionTests: XCTestCase { section.supportsRowSubtitles = false let items = section.items(for: state, now: Self.now) let iconBefore = items[4].image - XCTAssertNil(iconBefore, "precondition: the successful burst row is unmarked") + XCTAssertTrue(iconBefore === GlanceSection.clearRowImage, + "precondition: the successful burst row is visibly unmarked") state.glanceActivity = [ Self.entry(id: "o1", server: "obsidian", tool: "search_notes", status: "error", @@ -642,6 +643,24 @@ final class GlanceSectionTests: XCTestCase { ]) } + /// "Recent" and "Clients" are real section headers on macOS 14+, not + /// full-size disabled rows pretending to be — the system style is what + /// gives them the standard header margin and small grey type. Reverting + /// `sectionHeaderItem` to `disabledItem` keeps every title assertion green, + /// so the header-ness itself must be pinned. + func testRecentAndClientsAreSectionHeaders() throws { + guard #available(macOS 14.0, *) else { + throw XCTSkip("sectionHeader items exist on macOS 14+ only") + } + let section = Self.makeSection() + let items = section.items(for: Self.busyState(), now: Self.now) + + let recent = try XCTUnwrap(items.first { $0.title == "Recent" }) + let clients = try XCTUnwrap(items.first { $0.title == "Clients" }) + XCTAssertTrue(recent.isSectionHeader) + XCTAssertTrue(clients.isSectionHeader) + } + /// The histogram sits with the summary, above the separator that opens the /// detail — "directly below the summary line and above the Recent header" /// is a statement about neighbours, not merely about relative order. @@ -679,7 +698,8 @@ final class GlanceSectionTests: XCTestCase { XCTAssertEqual(row.title, "obsidian:search_notes — 5s") XCTAssertEqual(row.representedObject as? String, "sess-c", "the click payload must follow the title, or the row opens the previous record's session") - XCTAssertNil(row.image, "a successful row carries no mark") + XCTAssertTrue(row.image === GlanceSection.clearRowImage, + "a successful row carries no visible mark, only the alignment placeholder") XCTAssertEqual(row.toolTip, "obsidian:search_notes") XCTAssertEqual(row.accessibilityLabel(), "obsidian:search_notes, succeeded, 5s ago") } @@ -759,7 +779,8 @@ final class GlanceSectionTests: XCTestCase { section.supportsRowSubtitles = false let items = section.items(for: state, now: Self.now) let previousFailure = state.glanceActivity[1] - XCTAssertNil(items[4].image, "precondition: the successful row is unmarked") + XCTAssertTrue(items[4].image === GlanceSection.clearRowImage, + "precondition: the successful row is visibly unmarked") state.glanceActivity = [ Self.entry(id: "c", server: "obsidian", tool: "search_notes", status: "error", @@ -855,7 +876,11 @@ final class GlanceSectionTests: XCTestCase { let section = Self.makeSection() let row = section.items(for: Self.busyState(), now: Self.now)[4] - XCTAssertNil(row.image, "a quiet row is the whole point of failure-only marks") + XCTAssertTrue(row.image === GlanceSection.clearRowImage, + "a quiet row is the whole point of failure-only marks — the placeholder " + + "only reserves the icon column so titles align across the section") + XCTAssertNil(row.image?.accessibilityDescription, + "the placeholder must be silent for VoiceOver") XCTAssertEqual(row.accessibilityLabel(), "github:create_issue, succeeded, 30s ago") } @@ -910,7 +935,7 @@ final class GlanceSectionTests: XCTestCase { let section = Self.makeSection() let rows = Array(section.items(for: state, now: Self.now)[4...6]) - XCTAssertEqual(rows.map { $0.image == nil }, [true, false, true]) + XCTAssertEqual(rows.map { $0.image === GlanceSection.clearRowImage }, [true, false, true]) } /// US3 scenario 5: a burst of blocked attempts is one row with its count, @@ -932,7 +957,8 @@ final class GlanceSectionTests: XCTestCase { XCTAssertEqual(items[4].title, "jira:get_issue ×27 — 1s") XCTAssertEqual(items[4].image?.accessibilityDescription, "blocked") XCTAssertEqual(items[5].title, "jira:get_issue — 10m") - XCTAssertNil(items[5].image, "the successful calls are a separate, unmarked row") + XCTAssertTrue(items[5].image === GlanceSection.clearRowImage, + "the successful calls are a separate, visibly unmarked row") } // MARK: - Status is carried by shape AND colour diff --git a/native/macos/MCPProxy/MCPProxyTests/MainWindowRoutingTests.swift b/native/macos/MCPProxy/MCPProxyTests/MainWindowRoutingTests.swift new file mode 100644 index 000000000..8a9a3569d --- /dev/null +++ b/native/macos/MCPProxy/MCPProxyTests/MainWindowRoutingTests.swift @@ -0,0 +1,43 @@ +import XCTest +@testable import MCPProxy + +/// The native tab-switch contract between the tray and the main window. +/// +/// `showMainWindow(tab:)` has two paths — a fresh window seeds the sidebar +/// selection through `MainWindow(initialTab:)`, a live window receives a +/// `.switchToSidebarTab` notification — and the notification leg is stringly +/// typed (`SidebarItem` raw value as a String). These tests pin the decode +/// seam and the glance rows' destination so neither can silently rot. +@MainActor +final class MainWindowRoutingTests: XCTestCase { + + /// Every sidebar item survives the post-as-string / parse-on-receive + /// round trip that `showMainWindow(tab:)` and the window's `onReceive` + /// perform. + func testEverySidebarItemRoundTripsThroughTheNotification() { + for item in SidebarItem.allCases { + let note = Notification(name: .switchToSidebarTab, object: item.rawValue) + XCTAssertEqual(MainWindow.sidebarItem(from: note), item) + } + } + + /// A caller posting the SidebarItem itself (instead of its raw value) is + /// dropped, not crashed on — the silent-drop is the documented contract. + func testANonStringPayloadIsDropped() { + let wrongType = Notification(name: .switchToSidebarTab, object: SidebarItem.activity) + XCTAssertNil(MainWindow.sidebarItem(from: wrongType)) + + let unknown = Notification(name: .switchToSidebarTab, object: "No Such Section") + XCTAssertNil(MainWindow.sidebarItem(from: unknown)) + + let empty = Notification(name: .switchToSidebarTab, object: nil) + XCTAssertNil(MainWindow.sidebarItem(from: empty)) + } + + /// FR (menu QA 2026-08): a glance row or "Open Activity…" click must land + /// on the native Activity section, not the Web UI. The destination is a + /// constant precisely so this test exists without an app delegate. + func testGlanceRowsRouteToTheNativeActivitySection() { + XCTAssertEqual(AppController.glanceActivityDestination, .activity) + } +} From 3a3aac50be2f04b88fb14518c9f7523814159edd Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 5 Aug 2026 13:27:52 +0300 Subject: [PATCH 3/4] =?UTF-8?q?fix(telemetry):=20codex=20round=201=20?= =?UTF-8?q?=E2=80=94=20never=20echo=20rejected=20tpa=5Fscanner=20keys,=20r?= =?UTF-8?q?eject=20null=20objects?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The v8 anonymity rule copied an unknown tpa_scanner key into the violation Pattern, which sendHeartbeat logs — echoing the key there is itself the leak the rule exists to stop; the pattern is now constant. tpa_scanner:null and findings:null unmarshal into nil maps and slipped past the object-shape check; both are rejected explicitly. The Snapshot-to-Reset loss window is documented as the deliberate registry-wide semantic (shared by every counter; anonymous daily aggregates, seconds-long window). --- internal/telemetry/anonymity.go | 25 ++++++++++++++++++------- internal/telemetry/registry.go | 7 +++++++ internal/telemetry/tpa_scanner_test.go | 11 +++++++++-- 3 files changed, 34 insertions(+), 9 deletions(-) diff --git a/internal/telemetry/anonymity.go b/internal/telemetry/anonymity.go index 92b3d5e91..2c6803ba8 100644 --- a/internal/telemetry/anonymity.go +++ b/internal/telemetry/anonymity.go @@ -87,8 +87,11 @@ type anonymityScanEnvelope struct { LastErrorCode *json.RawMessage `json:"last_error_code"` // Schema v8 structural check: the security-scanner sub-object must be - // counts-and-fixed-enum-keys only. - TPAScanner *json.RawMessage `json:"tpa_scanner"` + // counts-and-fixed-enum-keys only. Deliberately NOT a pointer: JSON null + // sets a *RawMessage pointer to nil, which is indistinguishable from an + // absent field — as a plain RawMessage, absent stays empty while null + // arrives as the literal bytes "null" and fails the object-shape check. + TPAScanner json.RawMessage `json:"tpa_scanner"` } // v7FieldViolation builds the violation for a Spec 080 field that broke its @@ -229,12 +232,15 @@ var tpaScannerScalarKeys = []string{"scans_completed", "scans_failed", "scans_wi // This is the wire-form backstop for the producer-side filtering in // CounterRegistry.RecordTPAScanCompleted — a regression there (e.g. a server // name or rule id leaking in as a map key) is caught before transmit. -func scanV8TPAScanner(raw *json.RawMessage) *AnonymityViolation { - if raw == nil { +func scanV8TPAScanner(raw json.RawMessage) *AnonymityViolation { + if len(raw) == 0 { return nil } var obj map[string]json.RawMessage - if err := json.Unmarshal(*raw, &obj); err != nil { + // json.Unmarshal accepts `null` into a nil map, so nil-ness must be + // rejected explicitly — the field, when present, is required to be a + // real object. + if err := json.Unmarshal(raw, &obj); err != nil || obj == nil { return v8FieldViolation("tpa_scanner", "must be an object") } @@ -245,7 +251,10 @@ func scanV8TPAScanner(raw *json.RawMessage) *AnonymityViolation { allowed["findings"] = struct{}{} for k := range obj { if _, ok := allowed[k]; !ok { - return v8FieldViolation("tpa_scanner."+k, "is not a whitelisted key") + // The violation is logged on send failure — echoing the + // rejected key there would itself be the leak this rule + // exists to stop, so the pattern stays constant. + return v8FieldViolation("tpa_scanner", "carries a key outside the whitelist") } } @@ -265,7 +274,9 @@ func scanV8TPAScanner(raw *json.RawMessage) *AnonymityViolation { return nil } var findings map[string]json.RawMessage - if err := json.Unmarshal(rawFindings, &findings); err != nil { + // Same nil-map guard as the parent object: `findings: null` is not an + // object either. + if err := json.Unmarshal(rawFindings, &findings); err != nil || findings == nil { return v8FieldViolation("tpa_scanner.findings", "must be an object") } for sev, v := range findings { diff --git a/internal/telemetry/registry.go b/internal/telemetry/registry.go index 4acfdf9be..edff859a2 100644 --- a/internal/telemetry/registry.go +++ b/internal/telemetry/registry.go @@ -438,6 +438,13 @@ func (r *CounterRegistry) Snapshot() RegistrySnapshot { } // Reset zeros all counters. Called only after a successful heartbeat send. +// +// Deliberate, registry-wide trade-off: an event recorded between Snapshot() +// (payload build) and this Reset() (2xx received) is zeroed without ever +// being transmitted. The window is the seconds a daily send is in flight, +// the stats are anonymous aggregates, and the alternative — drain-and-restore +// on failure — buys nothing worth its complexity here. Every counter in this +// registry shares this semantic; do not "fix" it for one field. func (r *CounterRegistry) Reset() { for i := range r.surfaceCounts { r.surfaceCounts[i].Store(0) diff --git a/internal/telemetry/tpa_scanner_test.go b/internal/telemetry/tpa_scanner_test.go index e3591659c..b7f955933 100644 --- a/internal/telemetry/tpa_scanner_test.go +++ b/internal/telemetry/tpa_scanner_test.go @@ -380,6 +380,12 @@ func TestScanForPII_TPAScannerShapeViolations(t *testing.T) { `{"tpa_scanner":"github"}`}, {"findings not an object", `{"tpa_scanner":{"findings":["high"]}}`}, + // json.Unmarshal accepts null into a nil map, so null must be + // rejected explicitly — it is not the required object shape. + {"null tpa_scanner", + `{"tpa_scanner":null}`}, + {"null findings", + `{"tpa_scanner":{"scans_completed":1,"findings":null}}`}, } for _, tc := range dirty { err := ScanForPII([]byte(tc.payload)) @@ -396,8 +402,9 @@ func TestScanForPII_TPAScannerShapeViolations(t *testing.T) { t.Errorf("%s: rule = %q, want v8_field_invalid", tc.name, v.Rule) } // The violation must never echo the offending (possibly identifying) - // map key back into logs. - for _, secret := range []string{"TPA-2026-0001", "my-private-server", "github"} { + // map KEY or value back into logs — keys are where a server name + // would leak in, so "server_name" is in this list on purpose. + for _, secret := range []string{"TPA-2026-0001", "my-private-server", "github", "server_name"} { if strings.Contains(v.Pattern, secret) || strings.Contains(v.Reason, secret) { t.Errorf("%s: violation echoed %q back: pattern=%q reason=%q", tc.name, secret, v.Pattern, v.Reason) From 01a3ee51fbb1eb15988cf46f795d516c1296a675 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 5 Aug 2026 13:49:20 +0300 Subject: [PATCH 4/4] test(httpapi): track telemetry.SchemaVersion instead of pinning 7 The payload-endpoint test asserted schema_version == 7 literally; v8 (the tpa_scanner block) is additive and the v7 fields keep rendering, which is what the test actually pins. --- internal/httpapi/telemetry_payload_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/httpapi/telemetry_payload_test.go b/internal/httpapi/telemetry_payload_test.go index aeb0345fc..32efb54c6 100644 --- a/internal/httpapi/telemetry_payload_test.go +++ b/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"])