From a56ffa68dfda245e010202ea76f1ed02469c323e Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Wed, 1 Jul 2026 15:35:51 +0200 Subject: [PATCH] chore: daily-review follow-ups (MCP caps, embed norm assert, projection tests, Classify guard) Addresses four items from the daily code review, plus in-flight Web work and the .mcp.json endpoint switch to the custom domain. Review follow-ups: - MCP access-guard: bound BOTH rate-limiter maps (preAuthBuckets + rateBuckets) via boundedSet on insert (new MAX_RATE_BUCKET_ENTRIES, default 10_000). They previously only shrank via the 5-min reaper, so a distinct-key flood could grow them unbounded between passes. Adds getRateBucketSizes() + a flood-cap test. - MCP vector-search: assertUnitNorm() in embed()/index() (eps=1e-3, fail-loud). cosineSimilarity is a bare dot product that assumes L2-unit vectors; a violated normalize:true contract would otherwise silently corrupt ranking. +unit tests. - Backend: equivalence tests for the bcd307d SessionsIndex projections, after the b417963 pattern. Extract MetricsSummaryProjection/QuickSearchProjection named arrays + testable TallyMetricsSummaryRow/ReadQuickSearchIdentity helpers; make BuildQuickSearchResult internal. New Metrics/QuickSearch ProjectionEquivalence tests derive their keep-set from the production array (bite-verified: dropping Status/DeviceName fails them). - Agent V2 ModernDeploymentTracker: enforce the Classify() call-once invariant with a DEBUG-only [ThreadStatic] permit guard. Classify mutates the harmless- rollup counter, so a double-call double-counts; the invariant was comment-only. ProcessRecord/ProcessEvent issue one permit before their single call; Classify asserts + consumes it. Compiled out in RELEASE -> zero production impact. Also included (per request): - .mcp.json: point at https://mcp.autopilotmonitor.com/mcp (custom domain). - Web useAggregatedAdminScope hook + seed + tests (separate in-flight work). - Agent buildcounter.txt bumps. Tests: MCP 338, Backend 2551, V2.Core 1538, Web hooks 31 - all green. Co-Authored-By: Claude Opus 4.8 (1M context) --- .mcp.json | 2 +- .../SystemSignals/ModernDeploymentTracker.cs | 29 +++ .../buildcounter.txt | 2 +- .../AutopilotMonitor.Agent/buildcounter.txt | 2 +- ...etricsSummaryProjectionEquivalenceTests.cs | 140 +++++++++++++++ .../QuickSearchProjectionEquivalenceTests.cs | 141 +++++++++++++++ .../Services/TableStorageService.AgentApi.cs | 55 ++++-- .../src/__tests__/security-guards.test.ts | 17 +- .../__tests__/vector-search-provider.test.ts | 48 +++++ .../autopilot-monitor-mcp/src/access-guard.ts | 18 +- .../src/vector-search-provider.ts | 32 +++- .../__tests__/useAggregatedAdminScope.test.ts | 166 ++++++++++++++++++ .../hooks/aggregatedAdminScopeSeed.ts | 38 ++++ .../hooks/useAggregatedAdminScope.ts | 25 ++- 14 files changed, 689 insertions(+), 26 deletions(-) create mode 100644 src/Backend/AutopilotMonitor.Functions.Tests/MetricsSummaryProjectionEquivalenceTests.cs create mode 100644 src/Backend/AutopilotMonitor.Functions.Tests/QuickSearchProjectionEquivalenceTests.cs create mode 100644 src/McpServer/autopilot-monitor-mcp/src/__tests__/vector-search-provider.test.ts create mode 100644 src/Web/autopilot-monitor-web/hooks/__tests__/useAggregatedAdminScope.test.ts create mode 100644 src/Web/autopilot-monitor-web/hooks/aggregatedAdminScopeSeed.ts diff --git a/.mcp.json b/.mcp.json index 37ac1740..796fd59b 100644 --- a/.mcp.json +++ b/.mcp.json @@ -2,7 +2,7 @@ "mcpServers": { "autopilot-monitor": { "type": "http", - "url": "https://autopilotmonitor-mcp.kindwave-58b4b547.westeurope.azurecontainerapps.io/mcp" + "url": "https://mcp.autopilotmonitor.com/mcp" } } } diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Enrollment/SystemSignals/ModernDeploymentTracker.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Enrollment/SystemSignals/ModernDeploymentTracker.cs index a06240d3..126150a4 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Enrollment/SystemSignals/ModernDeploymentTracker.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core/Monitoring/Enrollment/SystemSignals/ModernDeploymentTracker.cs @@ -76,6 +76,18 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Enrollment.SystemSignals private readonly Dictionary _harmlessOccurrenceCounts = new Dictionary(); private readonly object _harmlessCountLock = new object(); +#if DEBUG + // Call-once invariant guard for Classify() (see its doc-comment): Classify mutates the + // harmless-rollup counter, so a second call for the SAME record would double-count. The + // two legitimate callers (ProcessRecord / ProcessEvent) set this permit immediately before + // their single Classify call; Classify asserts it is set and consumes it, so a second call + // within the same record's processing trips the assert — and a NEW caller that forgets to + // set the permit is caught too. [ThreadStatic] keeps it per-processing-thread so the + // concurrent EventRecordWritten callbacks (distinct records) never falsely collide. + [ThreadStatic] + private static bool s_classifyPermitted; +#endif + public ModernDeploymentTracker( string sessionId, string tenantId, @@ -231,6 +243,9 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Enrollment.SystemSignals // no EventID filter, so harmless-ID bursts (session 8bc1180f: ~689 EventID-100 // records/minute) would otherwise pay full formatting cost only to be discarded by // the rollup gate. Skip formatting entirely for rollup-suppressed records. +#if DEBUG + s_classifyPermitted = true; // single Classify permit for this record — see Classify(). +#endif var verdict = Classify(record.Id, record.Level, shortName, out var eventType, out var severity, out var harmlessDowngraded, out var occurrenceCount); if (verdict == EventVerdict.Suppress) @@ -269,6 +284,9 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Enrollment.SystemSignals string channelName, bool isBackfill) { +#if DEBUG + s_classifyPermitted = true; // single Classify permit for this record — see Classify(). +#endif var verdict = Classify(eventId, level, shortName, out var eventType, out var severity, out var harmlessDowngraded, out var occurrenceCount); if (verdict == EventVerdict.WhiteGlove) @@ -304,6 +322,17 @@ namespace AutopilotMonitor.Agent.V2.Core.Monitoring.Enrollment.SystemSignals out bool harmlessDowngraded, out int occurrenceCount) { +#if DEBUG + // Enforce the call-once invariant: the caller (ProcessRecord/ProcessEvent) must have + // just issued a single permit. Consume it so a second Classify for the same record trips. + System.Diagnostics.Debug.Assert( + s_classifyPermitted, + "ModernDeploymentTracker.Classify() called without a fresh per-record permit — it " + + "mutates the harmless-rollup counter and MUST be called exactly once per record " + + "(set s_classifyPermitted in ProcessRecord/ProcessEvent immediately before the call)."); + s_classifyPermitted = false; +#endif + eventType = null; severity = EventSeverity.Info; harmlessDowngraded = false; diff --git a/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt b/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt index 34a5f9bc..e944bea9 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt +++ b/src/Agent/AutopilotMonitor.Agent.V2/buildcounter.txt @@ -1 +1 @@ -1197 +1206 diff --git a/src/Agent/AutopilotMonitor.Agent/buildcounter.txt b/src/Agent/AutopilotMonitor.Agent/buildcounter.txt index 9c39429e..2df08016 100644 --- a/src/Agent/AutopilotMonitor.Agent/buildcounter.txt +++ b/src/Agent/AutopilotMonitor.Agent/buildcounter.txt @@ -1 +1 @@ -1481 +1483 diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/MetricsSummaryProjectionEquivalenceTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/MetricsSummaryProjectionEquivalenceTests.cs new file mode 100644 index 00000000..15cf1f52 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/MetricsSummaryProjectionEquivalenceTests.cs @@ -0,0 +1,140 @@ +using System; +using System.Collections.Generic; +using Azure.Data.Tables; +using AutopilotMonitor.Functions.Helpers; +using AutopilotMonitor.Functions.Services; +using Xunit; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// Pins that the column-projected SessionsIndex scan driving GetMetricsSummaryAsync +/// (bcd307d — select: MetricsSummaryProjection = { PartitionKey, Status }) tallies the +/// same per-tenant status buckets as the old full-row drain. +/// +/// In real Azure Table Storage a $select returns ONLY the projected properties (plus the +/// structural PartitionKey/RowKey); every other column is genuinely absent. These tests reproduce +/// that faithfully — the "projected" row keeps only +/// — then run the production tally () over +/// both shapes and assert identical buckets. Deriving the keep-set from the production array means +/// dropping a column there (e.g. Status) immediately fails these tests. +/// +public class MetricsSummaryProjectionEquivalenceTests +{ + private const string TenantA = "00000000-0000-0000-0000-00000000aaaa"; + private const string TenantB = "00000000-0000-0000-0000-00000000bbbb"; + + private static string IndexRowKey(DateTime startedAt, string sessionId) + => $"{(DateTime.MaxValue.Ticks - startedAt.Ticks):D19}_{sessionId}"; + + /// Full ~40-column mirror row as the cross-partition scan would read it (no projection). + private static TableEntity FullRow(string tenantId, string status, DateTime startedAt) + { + var sid = Guid.NewGuid().ToString(); + return new TableEntity(tenantId, IndexRowKey(startedAt, sid)) + { + ["SessionId"] = sid, + ["Status"] = status, + ["StartedAt"] = new DateTimeOffset(startedAt), + // Representative noise the tally never reads — present on a real mirror row, absent on + // the projected one. Proves the projection drops them without changing the outcome. + ["SerialNumber"] = "SN-FULL", + ["DeviceName"] = "PC-FULL", + ["OsName"] = "Windows 11", + ["GeoCountry"] = "DE", + ["EventCount"] = 123, + ["FailureSnapshotJson"] = "{\"big\":\"" + new string('x', 2000) + "\"}", + }; + } + + /// + /// Reduces a full mirror row to exactly what Azure returns under + /// $select = MetricsSummaryProjection: the projected columns plus the structural + /// PartitionKey/RowKey; every other property is genuinely absent. + /// + private static TableEntity Project(TableEntity full) + { + var keep = new HashSet(TableStorageService.MetricsSummaryProjection, StringComparer.Ordinal); + var projected = new TableEntity(full.PartitionKey, full.RowKey); + foreach (var kv in full) + { + if (kv.Key is "PartitionKey" or "RowKey" or "Timestamp" or "odata.etag") continue; + if (keep.Contains(kv.Key)) projected[kv.Key] = kv.Value; + } + return projected; + } + + private static Dictionary Tally(IEnumerable rows) + { + var groups = new Dictionary(StringComparer.OrdinalIgnoreCase); + foreach (var r in rows) TableStorageService.TallyMetricsSummaryRow(groups, r); + return groups; + } + + private static void AssertBucketsEqual(SessionStatusBuckets a, SessionStatusBuckets b) + { + Assert.Equal(a.Total, b.Total); + Assert.Equal(a.Succeeded, b.Succeeded); + Assert.Equal(a.Failed, b.Failed); + Assert.Equal(a.InProgress, b.InProgress); + Assert.Equal(a.Pending, b.Pending); + Assert.Equal(a.Stalled, b.Stalled); + Assert.Equal(a.Other, b.Other); + } + + [Fact] + public void Tally_is_identical_for_full_vs_projected_fleet() + { + var now = DateTime.UtcNow; + // A mixed multi-tenant fleet covering every bucket the summary reports. + var fleet = new (string Tenant, string Status)[] + { + (TenantA, "Succeeded"), + (TenantA, "Succeeded"), + (TenantA, "Failed"), + (TenantA, "InProgress"), + (TenantA, "Pending"), + (TenantA, "Stalled"), + (TenantA, "SomethingUnexpected"), // -> Other bucket + (TenantB, "Succeeded"), + (TenantB, "Failed"), + }; + + var fullRows = new List(); + var projectedRows = new List(); + var i = 0; + foreach (var (tenant, status) in fleet) + { + var full = FullRow(tenant, status, now.AddMinutes(-i++)); + fullRows.Add(full); + projectedRows.Add(Project(full)); + } + + var fullGroups = Tally(fullRows); + var projectedGroups = Tally(projectedRows); + + Assert.Equal(fullGroups.Count, projectedGroups.Count); + foreach (var tenant in fullGroups.Keys) + { + Assert.True(projectedGroups.ContainsKey(tenant)); + AssertBucketsEqual(fullGroups[tenant], projectedGroups[tenant]); + } + } + + [Fact] + public void Missing_status_column_defaults_to_inprogress_on_both_shapes() + { + // A mirror row can lack Status (defaults to "InProgress"). The projection keeps Status, so + // full and projected must agree — including on the missing-column default. + var row = new TableEntity(TenantA, IndexRowKey(DateTime.UtcNow, Guid.NewGuid().ToString())) + { + ["SerialNumber"] = "SN-NOSTATUS", + }; + + var full = Tally(new[] { row }); + var projected = Tally(new[] { Project(row) }); + + AssertBucketsEqual(full[TenantA], projected[TenantA]); + Assert.Equal(1, full[TenantA].InProgress); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/QuickSearchProjectionEquivalenceTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/QuickSearchProjectionEquivalenceTests.cs new file mode 100644 index 00000000..3d27e4c7 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/QuickSearchProjectionEquivalenceTests.cs @@ -0,0 +1,141 @@ +using System; +using System.Collections.Generic; +using Azure.Data.Tables; +using AutopilotMonitor.Functions.Services; +using AutopilotMonitor.Shared.Models; +using Microsoft.Extensions.Logging.Abstractions; +using Moq; +using Xunit; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// Pins that the column-projected SessionsIndex scan driving QuickSearchSessionsAsync +/// (bcd307d — select: QuickSearchProjection) resolves the same match identity and builds the +/// same as the old full-row scan. +/// +/// In real Azure Table Storage a $select returns ONLY the projected properties (plus the +/// structural PartitionKey/RowKey); every other column is genuinely absent. These tests reproduce +/// that faithfully — the "projected" row keeps only +/// — then run the production reads ( + +/// ) over both shapes and assert equivalence. +/// Deriving the keep-set from the production array means dropping a column there immediately fails +/// these tests (e.g. dropping "DeviceName" breaks the identity/result equivalence; dropping "RowKey" +/// breaks the SessionId fallback). +/// +public class QuickSearchProjectionEquivalenceTests +{ + private const string TenantId = "00000000-0000-0000-0000-000000000abc"; + + private static readonly TableStorageService Sut = + new(new Mock().Object, NullLogger.Instance); + + // SessionsIndex RowKey = inverted-tick prefix + "_" + sessionId (see ComputeIndexRowKey). + private static string IndexRowKey(DateTime startedAt, string sessionId) + => $"{(DateTime.MaxValue.Ticks - startedAt.Ticks):D19}_{sessionId}"; + + /// Full ~40-column mirror row as the unprojected scan would read it. + private static TableEntity FullRow(string sessionId, string serial, string deviceName, string status, DateTime startedAt) + { + return new TableEntity(TenantId, IndexRowKey(startedAt, sessionId)) + { + ["SessionId"] = sessionId, + ["SerialNumber"] = serial, + ["DeviceName"] = deviceName, + ["Status"] = status, + ["StartedAt"] = new DateTimeOffset(startedAt), + // Representative noise the typeahead never reads — present on a real mirror row, absent + // on the projected one. Proves the projection drops them without changing the outcome. + ["Manufacturer"] = "Contoso", + ["Model"] = "Model-X", + ["OsName"] = "Windows 11", + ["GeoCountry"] = "DE", + ["EventCount"] = 123, + ["FailureSnapshotJson"] = "{\"big\":\"" + new string('x', 2000) + "\"}", + }; + } + + /// + /// Reduces a full mirror row to exactly what Azure returns under + /// $select = QuickSearchProjection: the projected columns plus the structural + /// PartitionKey/RowKey; every other property is genuinely absent. + /// + private static TableEntity Project(TableEntity full) + { + var keep = new HashSet(TableStorageService.QuickSearchProjection, StringComparer.Ordinal); + var projected = new TableEntity(full.PartitionKey, full.RowKey); + foreach (var kv in full) + { + if (kv.Key is "PartitionKey" or "RowKey" or "Timestamp" or "odata.etag") continue; + if (keep.Contains(kv.Key)) projected[kv.Key] = kv.Value; + } + return projected; + } + + [Fact] + public void Identity_and_result_are_identical_for_full_vs_projected_row() + { + var started = DateTime.UtcNow.AddHours(-1); + var sid = Guid.NewGuid().ToString(); + var full = FullRow(sid, "SN-12345", "PC-ALICE", "Succeeded", started); + var projected = Project(full); + + // Match identity — the fields the typeahead matches on (SessionId, SerialNumber, DeviceName). + var idFull = TableStorageService.ReadQuickSearchIdentity(full); + var idProjected = TableStorageService.ReadQuickSearchIdentity(projected); + Assert.Equal(idFull, idProjected); + Assert.Equal(sid, idProjected.SessionId); + Assert.Equal("SN-12345", idProjected.Serial); + Assert.Equal("PC-ALICE", idProjected.DeviceName); + + // Result — the emitted DTO (Status + StartedAt read from the entity). + var resFull = Sut.BuildQuickSearchResult(full, idFull.SessionId, idFull.Serial, idFull.DeviceName, "serialNumber"); + var resProjected = Sut.BuildQuickSearchResult(projected, idProjected.SessionId, idProjected.Serial, idProjected.DeviceName, "serialNumber"); + + Assert.Equal(resFull.SessionId, resProjected.SessionId); + Assert.Equal(resFull.SerialNumber, resProjected.SerialNumber); + Assert.Equal(resFull.DeviceName, resProjected.DeviceName); + Assert.Equal(resFull.Status, resProjected.Status); + Assert.Equal(SessionStatus.Succeeded, resProjected.Status); + Assert.Equal(resFull.StartedAt, resProjected.StartedAt); + Assert.Equal(resFull.MatchedField, resProjected.MatchedField); + } + + [Fact] + public void SessionId_falls_back_to_the_RowKey_on_both_shapes_when_the_column_is_absent() + { + // A mirror row can lack the SessionId column; the scan then derives it from the RowKey. + // RowKey is in QuickSearchProjection precisely so this fallback stays identical — this test + // would fail if "RowKey" were ever dropped from the projection. + var started = DateTime.UtcNow.AddHours(-2); + var sid = Guid.NewGuid().ToString(); + var full = FullRow(sid, "SN-9", "PC-BOB", "InProgress", started); + full.Remove("SessionId"); + + var idFull = TableStorageService.ReadQuickSearchIdentity(full); + var idProjected = TableStorageService.ReadQuickSearchIdentity(Project(full)); + + Assert.Equal(sid, idFull.SessionId); // resolved from RowKey + Assert.Equal(idFull.SessionId, idProjected.SessionId); + } + + [Fact] + public void Missing_serial_and_device_collapse_to_empty_on_both_shapes() + { + var started = DateTime.UtcNow.AddHours(-3); + var sid = Guid.NewGuid().ToString(); + var row = new TableEntity(TenantId, IndexRowKey(started, sid)) + { + ["SessionId"] = sid, + ["Status"] = "Failed", + ["StartedAt"] = new DateTimeOffset(started), + }; + + var idFull = TableStorageService.ReadQuickSearchIdentity(row); + var idProjected = TableStorageService.ReadQuickSearchIdentity(Project(row)); + + Assert.Equal(idFull, idProjected); + Assert.Equal(string.Empty, idProjected.Serial); + Assert.Equal(string.Empty, idProjected.DeviceName); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.AgentApi.cs b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.AgentApi.cs index 998564d5..bff58d42 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.AgentApi.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.AgentApi.cs @@ -313,6 +313,15 @@ namespace AutopilotMonitor.Functions.Services /// Hard upper bound on index rows a single typeahead request may scan (cost backstop). private const int QuickSearchMaxScannedRows = 5000; + /// + /// Columns the typeahead scan projects: the three matched identity fields, Status + StartedAt + /// for the result, and RowKey for the SessionId fallback (). + /// internal so QuickSearchProjectionEquivalenceTests derives its keep-set from this + /// array — dropping a column here immediately fails the equivalence test. + /// + internal static readonly string[] QuickSearchProjection = + { "RowKey", "SessionId", "SerialNumber", "DeviceName", "Status", "StartedAt" }; + /// /// Lightweight typeahead search: matches SessionId, SerialNumber, or DeviceName. /// Supports exact contains match (priority) and fuzzy match (edit distance <= 2). @@ -342,14 +351,12 @@ namespace AutopilotMonitor.Functions.Services // ExtractSessionIdFromIndexRowKey). Avoids pulling the full ~40-column SessionsIndex // mirror for up to QuickSearchMaxScannedRows rows on every keystroke-driven request. await foreach (var entity in indexTableClient.QueryAsync( - filter: filter, select: new[] { "RowKey", "SessionId", "SerialNumber", "DeviceName", "Status", "StartedAt" })) + filter: filter, select: QuickSearchProjection)) { if (++scannedRows > QuickSearchMaxScannedRows) break; - var sessionId = entity.GetString("SessionId") ?? ExtractSessionIdFromIndexRowKey(entity.RowKey); - var serial = entity.GetString("SerialNumber") ?? string.Empty; - var deviceName = entity.GetString("DeviceName") ?? string.Empty; + var (sessionId, serial, deviceName) = ReadQuickSearchIdentity(entity); // Phase 1: exact substring match (highest priority) string? matchedField = null; @@ -387,7 +394,19 @@ namespace AutopilotMonitor.Functions.Services return exactResults.Count > limit ? exactResults.GetRange(0, limit) : exactResults; } - private QuickSearchResult BuildQuickSearchResult( + /// + /// Resolves the three identity fields a typeahead row matches on, applying the SessionId -> + /// RowKey fallback. internal + static so QuickSearchProjectionEquivalenceTests can drive + /// the exact production read against a full vs a projected row. + /// + internal static (string SessionId, string Serial, string DeviceName) ReadQuickSearchIdentity(TableEntity entity) + => (entity.GetString("SessionId") ?? ExtractSessionIdFromIndexRowKey(entity.RowKey), + entity.GetString("SerialNumber") ?? string.Empty, + entity.GetString("DeviceName") ?? string.Empty); + + // internal (not private) so QuickSearchProjectionEquivalenceTests can pin that a row carrying + // only QuickSearchProjection produces the same result (Status/StartedAt) as a full mirror row. + internal QuickSearchResult BuildQuickSearchResult( TableEntity entity, string sessionId, string serial, string deviceName, string matchedField) { var statusString = entity.GetString("Status") ?? "InProgress"; @@ -1246,6 +1265,25 @@ namespace AutopilotMonitor.Functions.Services /// Returns aggregated session metrics grouped by tenant, filtered to the last days. /// Leverages the SessionsIndex inverted-tick RowKey ordering for an efficient server-side range scan. /// + /// + /// Columns the status tally consumes: PartitionKey (grouping) + Status. internal so + /// MetricsSummaryProjectionEquivalenceTests derives its keep-set from this array. + /// + internal static readonly string[] MetricsSummaryProjection = { "PartitionKey", "Status" }; + + /// + /// Tally one SessionsIndex row into its tenant's status buckets. Reads ONLY the two + /// projected columns (PartitionKey + Status), so a projected row must produce identical + /// buckets to a full mirror row — pinned by MetricsSummaryProjectionEquivalenceTests. + /// + internal static void TallyMetricsSummaryRow(Dictionary groups, TableEntity entity) + { + var pk = entity.PartitionKey; + var statusStr = entity.GetString("Status") ?? "InProgress"; + groups.TryGetValue(pk, out var g); + groups[pk] = g.Add(statusStr); + } + public async Task> GetMetricsSummaryAsync(string? tenantId, int days = 30) { // Clamp to a sane range so callers can't accidentally trigger an unbounded scan. @@ -1269,12 +1307,9 @@ namespace AutopilotMonitor.Functions.Services // ~40-column mirror (incl. the large FailureSnapshotJson), so reading it whole for a // status count multiplied transfer ~10-20x for nothing on this cross-partition scan. await foreach (var entity in indexTableClient.QueryAsync( - filter: oDataFilter, select: new[] { "PartitionKey", "Status" })) + filter: oDataFilter, select: MetricsSummaryProjection)) { - var pk = entity.PartitionKey; - var statusStr = entity.GetString("Status") ?? "InProgress"; - groups.TryGetValue(pk, out var g); - groups[pk] = g.Add(statusStr); + TallyMetricsSummaryRow(groups, entity); } return groups.Select(kvp => (object)new diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts index 0f326cf2..8b9803d5 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/security-guards.test.ts @@ -298,7 +298,11 @@ describe('L3 — error-handler 5xx body redaction', () => { // module init) so the throttle tests are cheap and limit-independent. const PRE_AUTH_TEST_LIMIT = 5; process.env.MCP_PRE_AUTH_RATE_LIMIT_PER_MINUTE = String(PRE_AUTH_TEST_LIMIT); -const { buildCacheKey, boundedSet, isPreAuthRateLimited } = await import('../access-guard.js'); +// Pin a tiny rate-bucket cap (also read at module init) so the size-cap test can flood a +// handful of distinct keys instead of 10_000. Well above the per-UPN throttle tests' key count. +const RATE_BUCKET_TEST_CAP = 10; +process.env.MCP_RATE_BUCKET_MAX_ENTRIES = String(RATE_BUCKET_TEST_CAP); +const { buildCacheKey, boundedSet, isPreAuthRateLimited, getRateBucketSizes } = await import('../access-guard.js'); const { parsePositiveInt, getPublicBaseUrl } = await import('../config.js'); describe('F1 — access-guard cache key includes token hash', () => { @@ -539,6 +543,17 @@ describe('M1 — boundedSet size cap (accessCache memory bound)', () => { }); }); +describe('M1b — rate-bucket maps stay bounded under a distinct-key flood', () => { + // The rate buckets only shrink via the 5-min reaper, so a distinct-key flood + // (forged UPNs / NAT churn) must not grow them unbounded between passes. + it('caps the pre-auth (per-IP) bucket map at MCP_RATE_BUCKET_MAX_ENTRIES', () => { + for (let i = 0; i < RATE_BUCKET_TEST_CAP * 20; i++) { + isPreAuthRateLimited(`10.0.${Math.floor(i / 256)}.${i % 256}`); + } + expect(getRateBucketSizes().preAuth).toBeLessThanOrEqual(RATE_BUCKET_TEST_CAP); + }); +}); + describe('M1 — isPreAuthRateLimited (per-IP pre-auth throttle)', () => { // Budget pinned to PRE_AUTH_TEST_LIMIT above. Use a unique IP per test so the // module-level bucket map does not leak across cases. diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/vector-search-provider.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/vector-search-provider.test.ts new file mode 100644 index 00000000..d7a431a3 --- /dev/null +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/vector-search-provider.test.ts @@ -0,0 +1,48 @@ +/** + * Unit tests for the embedding unit-norm assertion. cosineSimilarity in + * vector-search-provider is a bare dot product that assumes L2-unit-normalized inputs, + * so embed()/index() assert the model actually returned a unit vector. These tests drive + * assertUnitNorm directly with canned vectors — no ML model load required. + */ +import { describe, it, expect } from 'vitest'; +import { assertUnitNorm } from '../vector-search-provider.js'; + +/** Build an L2-unit vector of the given dimension (all components equal). */ +function unitVector(dim: number): number[] { + const v = 1 / Math.sqrt(dim); + return new Array(dim).fill(v); +} + +describe('assertUnitNorm', () => { + it('accepts an exactly unit-normalized vector', () => { + expect(() => assertUnitNorm(unitVector(384), 'test')).not.toThrow(); + }); + + it('accepts a simple axis-aligned unit vector', () => { + expect(() => assertUnitNorm([1, 0, 0, 0], 'test')).not.toThrow(); + }); + + it('tolerates float32 rounding just inside epsilon', () => { + // norm ~= 1 + 5e-4, within the 1e-3 tolerance. + const v = unitVector(384).map((x) => x * (1 + 5e-4)); + expect(() => assertUnitNorm(v, 'test')).not.toThrow(); + }); + + it('throws on a non-normalized (un-scaled) vector', () => { + // A raw [1,1,1,1] has norm 2 — the exact failure mode a dropped `normalize: true` produces. + expect(() => assertUnitNorm([1, 1, 1, 1], 'test')).toThrow(/not L2-unit-normalized/); + }); + + it('throws on a vector whose norm drifts beyond epsilon', () => { + const v = unitVector(384).map((x) => x * 1.01); // norm ~= 1.01, outside 1e-3 + expect(() => assertUnitNorm(v, 'test')).toThrow(/not L2-unit-normalized/); + }); + + it('throws on a zero vector', () => { + expect(() => assertUnitNorm([0, 0, 0, 0], 'test')).toThrow(/not L2-unit-normalized/); + }); + + it('names the context in the error to aid diagnosis', () => { + expect(() => assertUnitNorm([5, 0], 'index:doc-42')).toThrow(/index:doc-42/); + }); +}); diff --git a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts b/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts index b716b9e6..f4706943 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/access-guard.ts @@ -41,6 +41,15 @@ const PRE_AUTH_RATE_LIMIT = parsePositiveInt(process.env.MCP_PRE_AUTH_RATE_LIMIT // container. Oldest entries are evicted on write once the cap is hit. const MAX_ACCESS_CACHE_ENTRIES = parsePositiveInt(process.env.MCP_ACCESS_CACHE_MAX_ENTRIES, 10_000); +// Hard ceiling on rate-bucket cardinality (both the per-UPN and per-source-IP maps). +// A bucket is created per distinct key on first sight and only removed by the 5-min +// reaper (empty-window prune), so a distinct-key flood — spoof-resistant IPs are hard, +// but forged UPNs and NAT churn are not — could grow either map unbounded between +// reaper passes. Same memory-exhaustion vector, same fix as the access cache: evict the +// oldest bucket on write once the cap is hit. The cap is far above any legitimate +// per-minute active-key count, so real callers are never evicted mid-window. +const MAX_RATE_BUCKET_ENTRIES = parsePositiveInt(process.env.MCP_RATE_BUCKET_MAX_ENTRIES, 10_000); + // Timeout for the backend access-check fetch. Without it a hung/cold backend // stalls the path EVERY request must traverse. Generous enough to survive a // cold Functions backend (→ no false 403s), bounded so an unresponsive backend @@ -258,7 +267,9 @@ function isWindowExceeded(buckets: Map, key: string, limit: n let entry = buckets.get(key); if (!entry) { entry = { timestamps: [] }; - buckets.set(key, entry); + // boundedSet caps the map so a distinct-key flood cannot grow it unbounded + // between the 5-min reaper passes (evicts the oldest bucket at the cap). + boundedSet(buckets, key, entry, MAX_RATE_BUCKET_ENTRIES); } entry.timestamps = entry.timestamps.filter((t) => t > windowStart); @@ -276,6 +287,11 @@ export function isPreAuthRateLimited(clientIp: string): boolean { return isWindowExceeded(preAuthBuckets, clientIp, PRE_AUTH_RATE_LIMIT); } +/** Current cardinality of the two rate-bucket maps. Exported for unit testing the size cap. */ +export function getRateBucketSizes(): { rate: number; preAuth: number } { + return { rate: rateBuckets.size, preAuth: preAuthBuckets.size }; +} + /** * Best-effort client IP for the pre-auth throttle. With `app.set('trust proxy', 1)` * (index.ts) and the single Container Apps Envoy ingress hop, req.ip is the real diff --git a/src/McpServer/autopilot-monitor-mcp/src/vector-search-provider.ts b/src/McpServer/autopilot-monitor-mcp/src/vector-search-provider.ts index 5f511d13..c6ecfff4 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/vector-search-provider.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/vector-search-provider.ts @@ -45,11 +45,37 @@ function getEmbedder(): Promise { return embedderPromise; } +/** + * Every embedding this module produces MUST be L2-unit-normalized: cosineSimilarity is a + * bare dot product (see below) that silently returns wrong scores for non-unit inputs. We + * pass `normalize: true` to the model, but that is an upstream contract we don't control — + * a library/model swap could break it. Assert it at creation so a violated assumption fails + * loud (one query / one bad build) instead of silently degrading ranking. The tolerance + * comfortably covers float32 rounding over 384 dims (~1e-5) while still catching an + * un-normalized vector (norm far from 1). + */ +const UNIT_NORM_EPSILON = 1e-3; + +export function assertUnitNorm(vec: number[], context: string): void { + let sumSq = 0; + for (let i = 0; i < vec.length; i++) sumSq += vec[i] * vec[i]; + const norm = Math.sqrt(sumSq); + if (Math.abs(norm - 1) > UNIT_NORM_EPSILON) { + throw new Error( + `[vector-search] ${context}: embedding is not L2-unit-normalized (norm=${norm.toFixed(6)}, ` + + `dim=${vec.length}). cosineSimilarity assumes unit vectors (dot product only) and would ` + + `otherwise return silently wrong scores.` + ); + } +} + /** Compute a normalized embedding for a single text. */ export async function embed(text: string): Promise { const model = await getEmbedder(); const output = await model(text, { pooling: 'mean', normalize: true }); - return Array.from(output.data as Float32Array); + const vec = Array.from(output.data as Float32Array); + assertUnitNorm(vec, 'embed'); + return vec; } // ── Helpers ────────────────────────────────────────────────── @@ -109,7 +135,9 @@ export class VectorSearchProvider implements SearchProvider { const embeddings = await Promise.all( batch.map(async (d) => { const out = await model(d.text, { pooling: 'mean', normalize: true }); - return Array.from(out.data as Float32Array); + const vec = Array.from(out.data as Float32Array); + assertUnitNorm(vec, `index:${d.id}`); + return vec; }) ); for (let j = 0; j < batch.length; j++) { diff --git a/src/Web/autopilot-monitor-web/hooks/__tests__/useAggregatedAdminScope.test.ts b/src/Web/autopilot-monitor-web/hooks/__tests__/useAggregatedAdminScope.test.ts new file mode 100644 index 00000000..9e832448 --- /dev/null +++ b/src/Web/autopilot-monitor-web/hooks/__tests__/useAggregatedAdminScope.test.ts @@ -0,0 +1,166 @@ +import { describe, expect, it } from "vitest"; +import { resolveDelegatedSeed, resolveGaSeed } from "../aggregatedAdminScopeSeed"; + +/** + * Tests for the pure seed-resolution core of useAggregatedAdminScope. The hook itself wires four React + * contexts (tenant / auth / admin-mode / tenant-list) and adjusts state during render; the interesting, + * regression-prone logic is the seed PRECEDENCE, which these resolvers isolate so it can be pinned without + * a DOM. Scenarios mirror the code-review follow-up: a GA/Reader defaulting to the aggregated view, a URL + * deep-link / persisted selection overriding that default, and a delegated caller being unaffected by it. + */ + +const OWN = "own-tenant-id"; +const OTHER = "other-tenant-id"; + +describe("resolveGaSeed", () => { + it("defaults a GA/Reader to the aggregated view ('') when defaultAggregated is set (audit page)", () => { + // No deep-link, nothing persisted → the page default takes over. With defaultAggregated the GA lands + // in the cross-tenant 'All tenants' aggregate instead of their own tenant. + expect( + resolveGaSeed({ + firstInit: true, + urlTenantId: undefined, + storedScope: null, + ownTenantId: OWN, + defaultAggregated: true, + }), + ).toBe(""); + }); + + it("defaults a GA/Reader to their own tenant when defaultAggregated is NOT set (other pages)", () => { + expect( + resolveGaSeed({ + firstInit: true, + urlTenantId: undefined, + storedScope: null, + ownTenantId: OWN, + defaultAggregated: false, + }), + ).toBe(OWN); + }); + + it("lets a first-init ?tenantId= deep-link win over the defaultAggregated default", () => { + expect( + resolveGaSeed({ + firstInit: true, + urlTenantId: OTHER, + storedScope: null, + ownTenantId: OWN, + defaultAggregated: true, + }), + ).toBe(OTHER); + }); + + it("lets the deep-link win over a persisted selection too (first-init precedence)", () => { + expect( + resolveGaSeed({ + firstInit: true, + urlTenantId: OTHER, + storedScope: "stored-tenant", + ownTenantId: OWN, + defaultAggregated: false, + }), + ).toBe(OTHER); + }); + + it("ignores the deep-link when it is NOT the first init (re-entering GA mode restores persisted scope)", () => { + // The URL seed expresses one-time intent on first load. On a later re-seed (e.g. toggling GA mode back + // on) the deep-link must not resurface and clobber what the user has since selected. + expect( + resolveGaSeed({ + firstInit: false, + urlTenantId: OTHER, + storedScope: "stored-tenant", + ownTenantId: OWN, + defaultAggregated: false, + }), + ).toBe("stored-tenant"); + }); + + it("honours a persisted selection over the page default", () => { + expect( + resolveGaSeed({ + firstInit: true, + urlTenantId: undefined, + storedScope: "stored-tenant", + ownTenantId: OWN, + defaultAggregated: true, + }), + ).toBe("stored-tenant"); + }); + + it("treats a persisted '' as an intentional aggregated selection, not 'unset'", () => { + // "" is a VALID persisted value for a GA (they chose the aggregate). It must be honoured verbatim and + // must not fall through to the own-tenant default — only a null (never-persisted) storage does that. + expect( + resolveGaSeed({ + firstInit: true, + urlTenantId: undefined, + storedScope: "", + ownTenantId: OWN, + defaultAggregated: false, + }), + ).toBe(""); + }); + + it("ignores an empty-string deep-link (falsy) and falls through to persisted/default", () => { + // urlTenantId of "" is not a real deep-link target; the truthiness guard skips it. + expect( + resolveGaSeed({ + firstInit: true, + urlTenantId: "", + storedScope: null, + ownTenantId: OWN, + defaultAggregated: true, + }), + ).toBe(""); + }); +}); + +describe("resolveDelegatedSeed", () => { + const MANAGED = ["t-a", "t-b", "t-c"]; + + it("has no defaultAggregated input — a delegated caller is never aggregated (always concrete tenant)", () => { + // Structural guarantee: the resolver's signature cannot express "aggregated", so the audit page's + // defaultAggregated flag can have no effect on a delegated ('MSP') admin. + const seed = resolveDelegatedSeed({ + storedScope: null, + managedTenantIds: MANAGED, + firstManagedTenantId: MANAGED[0], + }); + expect(seed).toBe("t-a"); + expect(seed).not.toBe(""); + }); + + it("reuses a persisted selection when it is still inside the managed set", () => { + expect( + resolveDelegatedSeed({ + storedScope: "t-b", + managedTenantIds: MANAGED, + firstManagedTenantId: MANAGED[0], + }), + ).toBe("t-b"); + }); + + it("falls back to the first managed tenant when the persisted selection left the managed set", () => { + // Defence in depth: a stored tenant that is no longer delegated to this admin must not leak through. + expect( + resolveDelegatedSeed({ + storedScope: "t-revoked", + managedTenantIds: MANAGED, + firstManagedTenantId: MANAGED[0], + }), + ).toBe("t-a"); + }); + + it("falls back to the first managed tenant when a GA's persisted aggregate ('') is inherited", () => { + // "" is falsy → treated as no usable stored value; a delegated caller can never sit on the aggregate. + expect( + resolveDelegatedSeed({ + storedScope: "", + managedTenantIds: MANAGED, + firstManagedTenantId: MANAGED[0], + }), + ).toBe("t-a"); + }); +}); diff --git a/src/Web/autopilot-monitor-web/hooks/aggregatedAdminScopeSeed.ts b/src/Web/autopilot-monitor-web/hooks/aggregatedAdminScopeSeed.ts new file mode 100644 index 00000000..00c4f1f6 --- /dev/null +++ b/src/Web/autopilot-monitor-web/hooks/aggregatedAdminScopeSeed.ts @@ -0,0 +1,38 @@ +/** + * Pure seed-resolution core for {@link useAggregatedAdminScope}. Kept free of any React / context imports + * so the seed PRECEDENCE (the regression-prone part of the hook's state machine) is unit-testable in the + * node test environment without a DOM. The hook wires the live inputs; these functions decide the value. + */ + +/** + * GA/Reader seed resolver. Precedence: a first-init `?tenantId=` deep-link wins, else the tab-persisted + * selection ("" = the aggregated view is a valid persisted value for a GA), else the page default — own + * tenant, or "" when {@link params.defaultAggregated} is set (the audit page). + */ +export function resolveGaSeed(params: { + firstInit: boolean; + urlTenantId: string | undefined; + storedScope: string | null; + ownTenantId: string; + defaultAggregated: boolean; +}): string { + const { firstInit, urlTenantId, storedScope, ownTenantId, defaultAggregated } = params; + if (firstInit && urlTenantId) return urlTenantId; + if (storedScope !== null) return storedScope; + return defaultAggregated ? "" : ownTenantId; +} + +/** + * Delegated ("MSP") seed resolver — a delegated caller is NEVER aggregated, so there is no + * `defaultAggregated` input here: the result is always a concrete managed tenant. Reuses the tab-persisted + * selection only when it is still inside the managed set, else falls back to the first managed tenant. + */ +export function resolveDelegatedSeed(params: { + storedScope: string | null; + managedTenantIds: string[]; + firstManagedTenantId: string; +}): string { + const { storedScope, managedTenantIds, firstManagedTenantId } = params; + const storedManaged = storedScope && managedTenantIds.includes(storedScope) ? storedScope : null; + return storedManaged ?? firstManagedTenantId; +} diff --git a/src/Web/autopilot-monitor-web/hooks/useAggregatedAdminScope.ts b/src/Web/autopilot-monitor-web/hooks/useAggregatedAdminScope.ts index cf5f88e0..cbfd064b 100644 --- a/src/Web/autopilot-monitor-web/hooks/useAggregatedAdminScope.ts +++ b/src/Web/autopilot-monitor-web/hooks/useAggregatedAdminScope.ts @@ -6,6 +6,7 @@ import { useAuth } from "@/contexts/AuthContext"; import { useAdminMode } from "@/hooks/useAdminMode"; import { useTenantList, type TenantInfo } from "@/hooks/useTenantList"; import { readTenantScope, writeTenantScope } from "@/utils/tenantScopeStorage"; +import { resolveDelegatedSeed, resolveGaSeed } from "@/hooks/aggregatedAdminScopeSeed"; export interface AggregatedAdminScope { /** @@ -106,12 +107,14 @@ export function useAggregatedAdminScope(opts?: { // GA/Reader seed precedence: a first-init ?tenantId= deep-link wins, else the tab-persisted selection // ("" = aggregated is valid for a GA), else the page default (own tenant, or "" when defaultAggregated). - const computeGaSeed = (firstInit: boolean): string => { - if (firstInit && urlTenantId) return urlTenantId; - const stored = readTenantScope(); - if (stored !== null) return stored; - return defaultAggregated ? "" : tenantId; - }; + const computeGaSeed = (firstInit: boolean): string => + resolveGaSeed({ + firstInit, + urlTenantId, + storedScope: readTenantScope(), + ownTenantId: tenantId, + defaultAggregated, + }); // Default the selection (done DURING RENDER — React's "adjust state when an input changes" escape hatch; // it converges, so no committed render keyed on scopeKey observes a transient wrong scope). @@ -119,9 +122,13 @@ export function useAggregatedAdminScope(opts?: { // Delegated: no aggregate. Seed from the persisted managed tenant (if still managed) or the first // managed tenant once the (scoped) list arrives; re-default if the selection falls outside the set. if (tenants.length > 0 && (!selectedTenantId || !tenants.some((t) => t.tenantId === selectedTenantId))) { - const stored = readTenantScope(); - const storedManaged = stored && tenants.some((t) => t.tenantId === stored) ? stored : null; - setSelectedRaw(storedManaged ?? tenants[0].tenantId); + setSelectedRaw( + resolveDelegatedSeed({ + storedScope: readTenantScope(), + managedTenantIds: tenants.map((t) => t.tenantId), + firstManagedTenantId: tenants[0].tenantId, + }) + ); if (!scopeInitialized) setScopeInitialized(true); if (prevIsGlobalAdmin !== isGlobalAdmin) setPrevIsGlobalAdmin(isGlobalAdmin); }