From 98fd173fb81a9088fe13ad73f2a9196323e924aa Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Fri, 14 Aug 2026 16:50:29 +0200 Subject: [PATCH] rules+backend+mcp: rule-verify sweep fixes (dead factor, {{token}} evidence, stats cap, list stringify) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Full verify pass over all 54 built-in rules (validate_rule sweep, 30d/7d get_rule_stats, test_analyze_rule spot checks). Fixes: - ANALYZE-APP-007 v1.1.0: declare enrollment_failed as a non-required condition — the exists-factor referenced an undeclared signal and could never apply. - ANALYZE-APP-014 v1.1.0: add non-required event_data conditions for espCategory/espErrorCode/topCandidatePackage/candidateCount/confidence so all explanation {{tokens}} resolve instead of rendering literally. - ANALYZE-ENRL-002 v1.2.0: capture session_timeout.timeoutHours so {{timeoutHours}} interpolates (verified via dry-run: renders "5"). - RuleEngine TryFlatLookup/ResolveDotPath: flatten IEnumerable data values to a comma-joined string — List fields (e.g. app_tracking_summary .pendingNames) stringified to "System.Collections.Generic.List`1[…]", breaking contains-matching and {{pendingNames}} interpolation. - GetRuleStatsAsync: raise the 500-row cap to 10000 and log when hit. The scan walks dates ascending, so the old cap silently dropped the NEWEST dates of a 30d window — newly added rules (e.g. ANALYZE-SEC-008) were entirely missing from global stats. - MCP validate_rule lint: accept event_data_array itemField as a {{token}} resolver — {{identity}} in ANALYZE-SEC-005/006 was a false positive. Backend 4157 tests green, MCP 548 green + tsc clean. Co-Authored-By: Claude Fable 5 --- rules/analyze/ANALYZE-APP-007.json | 9 +++ rules/analyze/ANALYZE-APP-014.json | 47 ++++++++++++- rules/analyze/ANALYZE-ENRL-002.json | 11 ++- rules/dist/analyze-rules.json | 67 ++++++++++++++++++- .../RuleEngineDataPathResolverTests.cs | 31 +++++++++ .../TableStorage/TableMetricsRepository.cs | 2 +- .../RuleEngine.ConditionEvaluators.cs | 22 +++++- .../Services/TableStorageService.Metrics.cs | 13 +++- .../src/__tests__/rule-validation.test.ts | 11 +++ .../src/rule-validation.ts | 4 ++ .../DataAccess/IMetricsRepository.cs | 5 +- 11 files changed, 211 insertions(+), 11 deletions(-) diff --git a/rules/analyze/ANALYZE-APP-007.json b/rules/analyze/ANALYZE-APP-007.json index b344cd3b..20beb5b8 100644 --- a/rules/analyze/ANALYZE-APP-007.json +++ b/rules/analyze/ANALYZE-APP-007.json @@ -5,6 +5,7 @@ "description": "Detects when the app tracking summary reports multiple failed app installations.", "severity": "high", "category": "apps", + "version": "1.1.0", "enabled": true, "baseConfidence": 75, "conditions": [ @@ -16,6 +17,14 @@ "operator": "gte", "value": "2", "required": true + }, + { + "signal": "enrollment_failed", + "source": "event_type", + "eventType": "enrollment_failed", + "operator": "exists", + "value": "", + "required": false } ], "confidenceFactors": [ diff --git a/rules/analyze/ANALYZE-APP-014.json b/rules/analyze/ANALYZE-APP-014.json index b8dec6f9..be4015c2 100644 --- a/rules/analyze/ANALYZE-APP-014.json +++ b/rules/analyze/ANALYZE-APP-014.json @@ -5,7 +5,7 @@ "description": "Detects when the Enrollment Status Page failed on the Apps subcategory and the agent's AppX deployment log scan identified a suspected MSIX/Store package. Store/MSIX apps install outside the Win32 pipeline, so the regular per-app tracking sees nothing — this correlation names the likely culprit.", "severity": "warning", "category": "apps", - "version": "1.0.0", + "version": "1.1.0", "enabled": true, "baseConfidence": 75, "preconditions": [ @@ -35,6 +35,51 @@ "operator": "regex", "value": "^0x80073[cd]", "required": false + }, + { + "signal": "appx_esp_category", + "source": "event_data", + "eventType": "esp_appx_failure_analysis", + "dataField": "espCategory", + "operator": "exists", + "value": "", + "required": false + }, + { + "signal": "appx_esp_errorcode", + "source": "event_data", + "eventType": "esp_appx_failure_analysis", + "dataField": "espErrorCode", + "operator": "exists", + "value": "", + "required": false + }, + { + "signal": "appx_top_candidate_package", + "source": "event_data", + "eventType": "esp_appx_failure_analysis", + "dataField": "topCandidatePackage", + "operator": "exists", + "value": "", + "required": false + }, + { + "signal": "appx_candidate_count", + "source": "event_data", + "eventType": "esp_appx_failure_analysis", + "dataField": "candidateCount", + "operator": "exists", + "value": "", + "required": false + }, + { + "signal": "appx_scan_confidence", + "source": "event_data", + "eventType": "esp_appx_failure_analysis", + "dataField": "confidence", + "operator": "exists", + "value": "", + "required": false } ], "explanation": "The ESP **{{espCategory}}/Apps** subcategory failed with HRESULT **{{espErrorCode}}** — an error from the AppX/MSIX deployment stack, i.e. a **Microsoft Store / MSIX app**, not a Win32 app. These apps install via the EnterpriseModernAppManagement CSP and never appear in the IME Win32 sidecar logs, which is why all tracked Win32/WinGet apps in this session can look green while ESP still reports an Apps failure.\n\nThe agent scanned the AppXDeploymentServer event log during the failure settle window and identified **{{topCandidatePackageName}}** (package `{{topCandidatePackage}}`) as the most likely culprit — its deployment error matches the ESP failure code ({{candidateCount}} candidate(s) total, confidence: {{confidence}}).\n\n**This is a correlation-based assessment, not a confirmed root cause** — validate against the AppXDeploymentServer/Operational event log on the device or in the diagnostics archive before acting.", diff --git a/rules/analyze/ANALYZE-ENRL-002.json b/rules/analyze/ANALYZE-ENRL-002.json index 781ed45a..44427a60 100644 --- a/rules/analyze/ANALYZE-ENRL-002.json +++ b/rules/analyze/ANALYZE-ENRL-002.json @@ -5,7 +5,7 @@ "description": "Detects when the enrollment session was force-failed by the server-side inactivity timeout (no agent telemetry for the configured SessionTimeoutHours window) without ever completing or explicitly failing.", "severity": "high", "category": "enrollment", - "version": "1.1.0", + "version": "1.2.0", "enabled": true, "baseConfidence": 80, "preconditions": [ @@ -31,6 +31,15 @@ "value": "", "required": true }, + { + "signal": "timeoutHours", + "source": "event_data", + "eventType": "session_timeout", + "dataField": "timeoutHours", + "operator": "exists", + "value": "", + "required": false + }, { "signal": "desktop_was_reached", "source": "event_type", diff --git a/rules/dist/analyze-rules.json b/rules/dist/analyze-rules.json index 6eaf19e5..0167e674 100644 --- a/rules/dist/analyze-rules.json +++ b/rules/dist/analyze-rules.json @@ -274,6 +274,7 @@ "description": "Detects when the app tracking summary reports multiple failed app installations.", "severity": "high", "category": "apps", + "version": "1.1.0", "enabled": true, "baseConfidence": 75, "conditions": [ @@ -285,6 +286,14 @@ "operator": "gte", "value": "2", "required": true + }, + { + "signal": "enrollment_failed", + "source": "event_type", + "eventType": "enrollment_failed", + "operator": "exists", + "value": "", + "required": false } ], "confidenceFactors": [ @@ -559,7 +568,7 @@ "description": "Detects when the Enrollment Status Page failed on the Apps subcategory and the agent's AppX deployment log scan identified a suspected MSIX/Store package. Store/MSIX apps install outside the Win32 pipeline, so the regular per-app tracking sees nothing — this correlation names the likely culprit.", "severity": "warning", "category": "apps", - "version": "1.0.0", + "version": "1.1.0", "enabled": true, "baseConfidence": 75, "preconditions": [ @@ -589,6 +598,51 @@ "operator": "regex", "value": "^0x80073[cd]", "required": false + }, + { + "signal": "appx_esp_category", + "source": "event_data", + "eventType": "esp_appx_failure_analysis", + "dataField": "espCategory", + "operator": "exists", + "value": "", + "required": false + }, + { + "signal": "appx_esp_errorcode", + "source": "event_data", + "eventType": "esp_appx_failure_analysis", + "dataField": "espErrorCode", + "operator": "exists", + "value": "", + "required": false + }, + { + "signal": "appx_top_candidate_package", + "source": "event_data", + "eventType": "esp_appx_failure_analysis", + "dataField": "topCandidatePackage", + "operator": "exists", + "value": "", + "required": false + }, + { + "signal": "appx_candidate_count", + "source": "event_data", + "eventType": "esp_appx_failure_analysis", + "dataField": "candidateCount", + "operator": "exists", + "value": "", + "required": false + }, + { + "signal": "appx_scan_confidence", + "source": "event_data", + "eventType": "esp_appx_failure_analysis", + "dataField": "confidence", + "operator": "exists", + "value": "", + "required": false } ], "explanation": "The ESP **{{espCategory}}/Apps** subcategory failed with HRESULT **{{espErrorCode}}** — an error from the AppX/MSIX deployment stack, i.e. a **Microsoft Store / MSIX app**, not a Win32 app. These apps install via the EnterpriseModernAppManagement CSP and never appear in the IME Win32 sidecar logs, which is why all tracked Win32/WinGet apps in this session can look green while ESP still reports an Apps failure.\n\nThe agent scanned the AppXDeploymentServer event log during the failure settle window and identified **{{topCandidatePackageName}}** (package `{{topCandidatePackage}}`) as the most likely culprit — its deployment error matches the ESP failure code ({{candidateCount}} candidate(s) total, confidence: {{confidence}}).\n\n**This is a correlation-based assessment, not a confirmed root cause** — validate against the AppXDeploymentServer/Operational event log on the device or in the diagnostics archive before acting.", @@ -1357,7 +1411,7 @@ "description": "Detects when the enrollment session was force-failed by the server-side inactivity timeout (no agent telemetry for the configured SessionTimeoutHours window) without ever completing or explicitly failing.", "severity": "high", "category": "enrollment", - "version": "1.1.0", + "version": "1.2.0", "enabled": true, "baseConfidence": 80, "preconditions": [ @@ -1383,6 +1437,15 @@ "value": "", "required": true }, + { + "signal": "timeoutHours", + "source": "event_data", + "eventType": "session_timeout", + "dataField": "timeoutHours", + "operator": "exists", + "value": "", + "required": false + }, { "signal": "desktop_was_reached", "source": "event_type", diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/RuleEngineDataPathResolverTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/RuleEngineDataPathResolverTests.cs index a88f9b04..88b84cbd 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/RuleEngineDataPathResolverTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/RuleEngineDataPathResolverTests.cs @@ -108,4 +108,35 @@ public class RuleEngineDataPathResolverTests Assert.Null(RuleEngine.ResolveDotPath(data, "scan_summary.critical_cves")); } + + [Fact] + public void TryFlatLookup_ListValue_FlattensToJoinedString() + { + // Regression: app_tracking_summary.pendingNames is a List after DataJson + // deserialization. ToString() on it yields the .NET type name, which broke both + // contains-matching and {{pendingNames}} interpolation in ANALYZE-APP-015 findings. + var data = new Dictionary + { + ["pendingNames"] = new List { "Contoso VPN", "Fabrikam Agent" }, + }; + + var ok = RuleEngine.TryFlatLookup(data, "pendingNames", out var value); + + Assert.True(ok); + Assert.Equal("Contoso VPN, Fabrikam Agent", value); + } + + [Fact] + public void ResolveDotPath_ListLeaf_FlattensToJoinedString() + { + var data = new Dictionary + { + ["summary"] = new Dictionary + { + ["names"] = new List { "a", "b" }, + }, + }; + + Assert.Equal("a, b", RuleEngine.ResolveDotPath(data, "summary.names")); + } } diff --git a/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableMetricsRepository.cs b/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableMetricsRepository.cs index f3c6c852..fb7ba235 100644 --- a/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableMetricsRepository.cs +++ b/src/Backend/AutopilotMonitor.Functions/DataAccess/TableStorage/TableMetricsRepository.cs @@ -106,7 +106,7 @@ namespace AutopilotMonitor.Functions.DataAccess.TableStorage => _storage.SaveRuleStatsEntryAsync(entry); public Task> GetRuleStatsAsync(string? tenantId = null, string? startDate = null, - string? endDate = null, string? ruleType = null, int maxResults = 500) + string? endDate = null, string? ruleType = null, int maxResults = 10000) => _storage.GetRuleStatsAsync(tenantId, startDate, endDate, ruleType, maxResults); public Task DeleteRuleStatsOlderThanAsync(DateTime cutoffDate) diff --git a/src/Backend/AutopilotMonitor.Functions/Services/RuleEngine.ConditionEvaluators.cs b/src/Backend/AutopilotMonitor.Functions/Services/RuleEngine.ConditionEvaluators.cs index e280fa55..ef763463 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/RuleEngine.ConditionEvaluators.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/RuleEngine.ConditionEvaluators.cs @@ -584,19 +584,35 @@ namespace AutopilotMonitor.Functions.Services { if (data.TryGetValue(key, out var raw)) { - value = raw?.ToString(); + value = StringifyDataValue(raw); return true; } var hit = data.Keys.FirstOrDefault(k => k.Equals(key, StringComparison.OrdinalIgnoreCase)); if (hit != null) { - value = data[hit]?.ToString(); + value = StringifyDataValue(data[hit]); return true; } value = null; return false; } + /// + /// Collections deserialized from event DataJson (e.g. app_tracking_summary.pendingNames) + /// would otherwise stringify to their .NET type name ("System.Collections.Generic.List`1[…]"), + /// which breaks both operator matching (contains/regex) and {{token}} interpolation of the + /// evidence value. Flatten them to a comma-joined string; scalars keep plain ToString(). + /// + internal static string? StringifyDataValue(object? raw) + { + if (raw == null) return null; + if (raw is string s) return s; + if (raw is System.Collections.IDictionary) return raw.ToString(); + if (raw is System.Collections.IEnumerable seq) + return string.Join(", ", seq.Cast().Select(o => o?.ToString())); + return raw.ToString(); + } + internal static string? ResolveDotPath(Dictionary root, string path) { var parts = path.Split('.'); @@ -649,7 +665,7 @@ namespace AutopilotMonitor.Functions.Services { null => null, JsonElement jel => jel.ValueKind == JsonValueKind.String ? jel.GetString() : jel.ToString(), - _ => current.ToString(), + _ => StringifyDataValue(current), }; } diff --git a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Metrics.cs b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Metrics.cs index d2517be4..1566d5d8 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Metrics.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Metrics.cs @@ -1377,7 +1377,7 @@ namespace AutopilotMonitor.Functions.Services /// public async Task> GetRuleStatsAsync( string? tenantId = null, string? startDate = null, string? endDate = null, - string? ruleType = null, int maxResults = 500) + string? ruleType = null, int maxResults = 10000) { try { @@ -1390,7 +1390,16 @@ namespace AutopilotMonitor.Functions.Services await foreach (var entity in query) { results.Add(MapToRuleStatsEntry(entity)); - if (results.Count >= maxResults) break; + if (results.Count >= maxResults) + { + // The scan walks PartitionKey (= date) ascending, so hitting the cap + // drops the newest dates — callers would see recently added rules as + // missing rather than a truncated window. Surface it loudly. + _logger.LogWarning( + "Rule stats query hit the {MaxResults}-row cap for {TenantId} ({StartDate}..{EndDate}, {RuleType}) — newest dates are missing from the result", + maxResults, tenantId ?? "(all)", startDate, endDate, ruleType ?? "(all)"); + break; + } } return results; diff --git a/src/McpServer/autopilot-monitor-mcp/src/__tests__/rule-validation.test.ts b/src/McpServer/autopilot-monitor-mcp/src/__tests__/rule-validation.test.ts index 14e1eade..bcd81958 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/__tests__/rule-validation.test.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/__tests__/rule-validation.test.ts @@ -384,6 +384,17 @@ describe('analyze semantic lint', () => { const bad = validAnalyze(); bad.explanation = 'Value is {{nonexistent_token}}.'; expect(warnings(validateRuleDraft(bad).findings).some((m) => m.includes('nonexistent_token'))).toBe(true); + + // event_data_array evidence carries the matched element under field=itemField, so the + // engine interpolates {{itemField}} at runtime — the lint must not flag it (SEC-005/006). + const arrayRule = validAnalyze(); + (arrayRule.conditions as Array>)[0] = { + signal: 'ppkg_not_allowlisted', source: 'event_data_array', eventType: 'provisioning_package_scan', + dataField: 'artifacts', itemField: 'identity', operator: 'not_regex', + value: '^(?:Microsoft\\.Windows\\.Cosa)\\b', required: true, + }; + arrayRule.explanation = 'Unexpected package: {{identity}}'; + expect(warnings(validateRuleDraft(arrayRule).findings).some((m) => m.includes('{{identity}}'))).toBe(false); }); it('markSessionAsFailedDefault=true yields an info finding', () => { diff --git a/src/McpServer/autopilot-monitor-mcp/src/rule-validation.ts b/src/McpServer/autopilot-monitor-mcp/src/rule-validation.ts index 137a2d37..31332c1a 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/rule-validation.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/rule-validation.ts @@ -232,6 +232,7 @@ interface DraftCondition { source?: string; eventType?: string; dataField?: string; + itemField?: string; operator?: string; value?: string; required?: boolean; @@ -360,6 +361,9 @@ function lintAnalyzeRule(rule: Record): ValidationFinding[] { for (const c of conditions) { if (c.dataField) resolvable.add(c.dataField); if (c.signal) resolvable.add(c.signal); + // event_data_array evidence carries the matched element under field=itemField, + // so {{itemField}} interpolates at runtime (see EvaluateEventDataArrayCondition). + if (c.itemField) resolvable.add(c.itemField); } for (const token of extractTokens(rule)) { if (!resolvable.has(token)) { diff --git a/src/Shared/AutopilotMonitor.Shared/DataAccess/IMetricsRepository.cs b/src/Shared/AutopilotMonitor.Shared/DataAccess/IMetricsRepository.cs index 0f4877f9..8d2c7e61 100644 --- a/src/Shared/AutopilotMonitor.Shared/DataAccess/IMetricsRepository.cs +++ b/src/Shared/AutopilotMonitor.Shared/DataAccess/IMetricsRepository.cs @@ -92,8 +92,11 @@ namespace AutopilotMonitor.Shared.DataAccess Task IncrementRuleStatAsync(string date, string tenantId, string ruleId, string ruleType, string ruleTitle, string category, string severity, bool fired, int? confidenceScore); Task SaveRuleStatsEntryAsync(RuleStatsEntry entry); + // maxResults is a runaway backstop, not a page size: a 31-day window easily holds + // 1400+ per-day rows (rules × days), and the scan runs date-ascending, so a low cap + // silently drops the NEWEST dates — newly added rules then vanish from stats entirely. Task> GetRuleStatsAsync(string? tenantId = null, string? startDate = null, - string? endDate = null, string? ruleType = null, int maxResults = 500); + string? endDate = null, string? ruleType = null, int maxResults = 10000); Task DeleteRuleStatsOlderThanAsync(DateTime cutoffDate); // --- F1 Time Attribution (insights spec §F1, PR2) ---