diff --git a/docs/rules/analyze-rule-triggers.md b/docs/rules/analyze-rule-triggers.md index 5b0257e2..4f1783ba 100644 --- a/docs/rules/analyze-rule-triggers.md +++ b/docs/rules/analyze-rule-triggers.md @@ -86,7 +86,7 @@ Interim evaluation changes what a rule's conditions mean mid-flight. Three new l # Cost & Safety -- Interim runs are bounded by trigger frequency, and the chosen v1 triggers are rare by construction (`hybrid_login_pending` ≤1 per boot, `whiteglove_sealed` once per session). A tenant authoring `on_event:performance_snapshot` would be expensive — the trigger registry intersection happens per ingest batch, so worst case is one envelope per batch; if that ever hurts, a per-(session, reason) throttle marker is the v2 lever. The lint should warn on high-frequency trigger types (catalog already knows them). +- Interim runs are bounded by trigger frequency, and the chosen v1 triggers are rare by construction (`hybrid_login_pending` ≤1 per boot, `whiteglove_sealed` once per session). High-frequency telemetry event types (performance_snapshot, agent_metrics_snapshot, download_progress, network churn, log_entry, agent_trace, stall_probe_check) are **hard-blocked** as `on_event` triggers, not merely advised against: the CRUD write is rejected (`AnalyzeRuleTriggers.BlockedOnEventTypes`, code-only enforcement), the runtime matcher drops them from any row that slipped in (defense in depth — the trigger registry and the engine both go through `OnEventTypes`), and the MCP lint reports them as an error. The JSON mirror lives in `rules/guardrails.json` (`blockedInterimTriggerEventTypes`) with a parity test pinning it to the C# set — the same pattern the gather hard blocks use. If a legitimate rare-but-bursty type ever hurts, a per-(session, reason) throttle marker is the v2 lever. - Each run loads the full event stream once (existing engine behavior, no change); rule filtering makes interim passes strictly cheaper than terminal ones. - Queue semantics (retry via visibility timeout, poison after MaxDequeueCount, idempotent partial-success re-runs) are inherited unchanged; upserts keep idempotency. diff --git a/rules/guardrails.json b/rules/guardrails.json index db8bac30..0072a443 100644 --- a/rules/guardrails.json +++ b/rules/guardrails.json @@ -272,5 +272,15 @@ "Format-Volume", "Set-ExecutionPolicy" ], - "maxCommandLength": 2000 + "maxCommandLength": 2000, + "blockedInterimTriggerEventTypes": [ + "performance_snapshot", + "agent_metrics_snapshot", + "download_progress", + "network_state_change", + "network_connectivity_check", + "log_entry", + "agent_trace", + "stall_probe_check" + ] } diff --git a/rules/schema/analyze-rule.schema.json b/rules/schema/analyze-rule.schema.json index 6e623314..5899e257 100644 --- a/rules/schema/analyze-rule.schema.json +++ b/rules/schema/analyze-rule.schema.json @@ -77,7 +77,7 @@ }, "evaluateOn": { "type": "array", - "description": "When this rule is evaluated. Absent = [\"enrollment_end\"] (terminal-only, the historical behavior). Interim triggers let the rule fire before the session is terminal: \"whiteglove_sealed\" (first genuine WhiteGlove seal) and \"on_event:\" (an ingest batch contained that event type). Interim runs suppress markSessionAsFailed and record no stats; findings notify once per (session, rule). Interim-enabled rules need monotonic conditions — a not_exists precondition on enrollment_complete/enrollment_failed/session_timeout passes trivially mid-run.", + "description": "When this rule is evaluated. Absent = [\"enrollment_end\"] (terminal-only, the historical behavior). Interim triggers let the rule fire before the session is terminal: \"whiteglove_sealed\" (first genuine WhiteGlove seal) and \"on_event:\" (an ingest batch contained that event type). High-frequency telemetry event types are HARD-BLOCKED as on_event triggers (guardrails blockedInterimTriggerEventTypes): the backend rejects them on save and the runtime ignores them. Interim runs suppress markSessionAsFailed and record no stats; findings notify once per (session, rule). Interim-enabled rules need monotonic conditions — a not_exists precondition on enrollment_complete/enrollment_failed/session_timeout passes trivially mid-run.", "items": { "type": "string", "pattern": "^(enrollment_end|whiteglove_sealed|on_event:[a-z0-9_]{1,128})$" diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/RuleEngineEvaluateOnTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/RuleEngineEvaluateOnTests.cs index 73ada0e5..4bc2af34 100644 --- a/src/Backend/AutopilotMonitor.Functions.Tests/RuleEngineEvaluateOnTests.cs +++ b/src/Backend/AutopilotMonitor.Functions.Tests/RuleEngineEvaluateOnTests.cs @@ -328,7 +328,7 @@ public class RuleEngineEvaluateOnTests } [Fact] - public void BuiltIn_catalog_evaluateOn_triggers_are_all_grammatical() + public void BuiltIn_catalog_evaluateOn_triggers_are_all_grammatical_and_unblocked() { foreach (var rule in BuiltInAnalyzeRules.GetAll()) { @@ -338,10 +338,74 @@ public class RuleEngineEvaluateOnTests { Assert.True(AnalyzeRuleTriggers.IsValidTrigger(trigger), $"Rule {rule.RuleId} ships invalid evaluateOn trigger '{trigger}'"); + + if (trigger.StartsWith("on_event:", StringComparison.OrdinalIgnoreCase)) + { + var eventType = trigger.Substring("on_event:".Length); + Assert.False(AnalyzeRuleTriggers.IsBlockedOnEventType(eventType), + $"Rule {rule.RuleId} ships hard-blocked evaluateOn trigger '{trigger}' — the runtime would silently drop it"); + } } } } + // ── Hard-blocked high-frequency trigger types ─────────────────────────── + + [Fact] + public void Blocked_on_event_types_are_dropped_by_the_runtime_matcher() + { + // Defense in depth: even a persisted row carrying a blocked trigger is inert. + var rule = new AnalyzeRule + { + EvaluateOn = new List { "on_event:performance_snapshot", $"on_event:{ProbeEventType}" }, + }; + Assert.Equal(new[] { ProbeEventType }, AnalyzeRuleTriggers.OnEventTypes(rule)); + Assert.False(AnalyzeRuleTriggers.MatchesOnEvent(rule, new[] { "performance_snapshot" })); + } + + [Fact] + public async Task Create_rule_with_blocked_on_event_trigger_is_rejected() + { + var repo = new Mock(); + repo.Setup(r => r.GetAnalyzeRulesAsync(It.IsAny())).ReturnsAsync(new List()); + repo.Setup(r => r.GetRuleStatesAsync(It.IsAny())).ReturnsAsync(new Dictionary()); + repo.Setup(r => r.AnalyzeRuleExistsAsync(It.IsAny(), It.IsAny())).ReturnsAsync(false); + var service = new AnalyzeRuleService(repo.Object, NullLogger.Instance); + + var rule = ProbeRule("ANALYZE-CUSTOM-901", + evaluateOn: new List { "enrollment_end", "on_event:performance_snapshot" }); + rule.IsBuiltIn = false; + + var ex = await Assert.ThrowsAsync(() => service.CreateRuleAsync(TenantId, rule)); + Assert.Contains("blocked", ex.Message, StringComparison.OrdinalIgnoreCase); + Assert.Contains("performance_snapshot", ex.Message); + repo.Verify(r => r.StoreAnalyzeRuleAsync(It.IsAny(), It.IsAny()), Times.Never); + } + + [Fact] + public void Blocked_set_matches_the_guardrails_json_mirror() + { + // Enforcement is code-only (AnalyzeRuleTriggers.BlockedOnEventTypes); the JSON mirror + // feeds the MCP validate_rule pre-flight. This parity test keeps the two from drifting — + // an addition on either side fails here until the other side follows. + var guardrailsPath = Path.Combine(FindRepoRoot(), "rules", "guardrails.json"); + var json = Newtonsoft.Json.Linq.JObject.Parse(File.ReadAllText(guardrailsPath)); + var mirror = json["blockedInterimTriggerEventTypes"]!.ToObject()!; + + Assert.Equal( + AnalyzeRuleTriggers.BlockedOnEventTypes.OrderBy(t => t, StringComparer.Ordinal), + mirror.OrderBy(t => t, StringComparer.Ordinal)); + } + + private static string FindRepoRoot() + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir != null && !File.Exists(Path.Combine(dir.FullName, "AutopilotMonitor.sln"))) + dir = dir.Parent; + Assert.NotNull(dir); + return dir!.FullName; + } + // ── Fixture ───────────────────────────────────────────────────────────── private sealed record Fixture( diff --git a/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs b/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs index 03a982f4..326d8a5c 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs @@ -638,6 +638,22 @@ namespace AutopilotMonitor.Functions.Services $"Invalid evaluateOn trigger(s): {string.Join(", ", invalid)}. " + "Allowed: 'enrollment_end', 'whiteglove_sealed', 'on_event:' (lowercase snake_case)."); } + + // Hard block, not a hint: high-frequency telemetry event types would enqueue an + // interim analyze run on effectively every ingest batch. The runtime additionally + // ignores blocked types (AnalyzeRuleTriggers.OnEventTypes), but rejecting the write + // keeps the author from persisting a trigger that silently never fires. + var blocked = rule.EvaluateOn + .Where(t => t != null && t.StartsWith(AnalyzeRuleTriggers.OnEventPrefix, StringComparison.OrdinalIgnoreCase)) + .Select(t => t!.Substring(AnalyzeRuleTriggers.OnEventPrefix.Length).Trim()) + .Where(AnalyzeRuleTriggers.IsBlockedOnEventType) + .ToList(); + if (blocked.Count > 0) + { + throw new InvalidOperationException( + $"evaluateOn on_event trigger(s) blocked for high-frequency telemetry event type(s): {string.Join(", ", blocked)}. " + + "These occur on nearly every ingest batch and would run analysis continuously — pick a rare, problem-indicating event type."); + } } /// 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 dd3b991e..14e1eade 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 @@ -421,14 +421,23 @@ describe('evaluateOn (interim evaluation) lint', () => { expect(warnings(validateRuleDraft(terminalOnly).findings).some((m) => m.includes('passes trivially mid-run'))).toBe(false); }); - it('unknown on_event types warn, high-frequency types warn explicitly', () => { + it('unknown on_event types warn, hard-blocked high-frequency types are ERRORS', () => { const unknown = validAnalyze(); unknown.evaluateOn = ['enrollment_end', 'on_event:made_up_event']; expect(warnings(validateRuleDraft(unknown).findings).some((m) => m.includes('made_up_event'))).toBe(true); - const hot = validAnalyze(); - hot.evaluateOn = ['enrollment_end', 'on_event:performance_snapshot']; - expect(warnings(validateRuleDraft(hot).findings).some((m) => m.includes('high-frequency'))).toBe(true); + // Every blocked type from the guardrails mirror must produce an ERROR (valid=false) — + // a warning would not stop an end user. + for (const blocked of [ + 'performance_snapshot', 'agent_metrics_snapshot', 'download_progress', 'network_state_change', + 'network_connectivity_check', 'log_entry', 'agent_trace', 'stall_probe_check', + ]) { + const hot = validAnalyze(); + hot.evaluateOn = ['enrollment_end', `on_event:${blocked}`]; + const r = validateRuleDraft(hot); + expect(r.valid, blocked).toBe(false); + expect(errors(r.findings).some((m) => m.includes('HARD-BLOCKED')), blocked).toBe(true); + } }); it('KO with interim-only triggers warns (the KO would never apply)', () => { diff --git a/src/McpServer/autopilot-monitor-mcp/src/rule-authoring-guide.ts b/src/McpServer/autopilot-monitor-mcp/src/rule-authoring-guide.ts index 239c73ef..7b4117ba 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/rule-authoring-guide.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/rule-authoring-guide.ts @@ -220,8 +220,10 @@ export const RULE_AUTHORING_GUIDE = { 'WHEN the rule is evaluated. Absent = ["enrollment_end"] — the terminal-only default. ' + 'Interim triggers evaluate the rule BEFORE the session is terminal: "whiteglove_sealed" ' + '(first genuine WhiteGlove seal, technician still at the bench) and "on_event:" ' + - '(an ingest batch contained that event type; pick a rare, problem-indicating type — never ' + - 'high-frequency telemetry). Interim semantics: markSessionAsFailed is SUPPRESSED, no stats ' + + '(an ingest batch contained that event type; pick a rare, problem-indicating type — ' + + 'high-frequency telemetry types are HARD-BLOCKED: the backend rejects them on save and the ' + + 'runtime ignores them, see guardrails blockedInterimTriggerEventTypes). Interim semantics: ' + + 'markSessionAsFailed is SUPPRESSED, no stats ' + 'are recorded, findings notify once per (session, rule) and render as "preliminary" until ' + 'the enrollment-end pass finalizes or resolves them. CRITICAL: interim-enabled rules need ' + 'MONOTONIC conditions — a not_exists precondition on enrollment_complete / enrollment_failed ' + diff --git a/src/McpServer/autopilot-monitor-mcp/src/rule-authoring.generated.ts b/src/McpServer/autopilot-monitor-mcp/src/rule-authoring.generated.ts index 7e233500..ea19b8b8 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/rule-authoring.generated.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/rule-authoring.generated.ts @@ -319,7 +319,7 @@ export const ANALYZE_RULE_SCHEMA: Record = { }, "evaluateOn": { "type": "array", - "description": "When this rule is evaluated. Absent = [\"enrollment_end\"] (terminal-only, the historical behavior). Interim triggers let the rule fire before the session is terminal: \"whiteglove_sealed\" (first genuine WhiteGlove seal) and \"on_event:\" (an ingest batch contained that event type). Interim runs suppress markSessionAsFailed and record no stats; findings notify once per (session, rule). Interim-enabled rules need monotonic conditions — a not_exists precondition on enrollment_complete/enrollment_failed/session_timeout passes trivially mid-run.", + "description": "When this rule is evaluated. Absent = [\"enrollment_end\"] (terminal-only, the historical behavior). Interim triggers let the rule fire before the session is terminal: \"whiteglove_sealed\" (first genuine WhiteGlove seal) and \"on_event:\" (an ingest batch contained that event type). High-frequency telemetry event types are HARD-BLOCKED as on_event triggers (guardrails blockedInterimTriggerEventTypes): the backend rejects them on save and the runtime ignores them. Interim runs suppress markSessionAsFailed and record no stats; findings notify once per (session, rule). Interim-enabled rules need monotonic conditions — a not_exists precondition on enrollment_complete/enrollment_failed/session_timeout passes trivially mid-run.", "items": { "type": "string", "pattern": "^(enrollment_end|whiteglove_sealed|on_event:[a-z0-9_]{1,128})$" @@ -1034,5 +1034,15 @@ export const RULE_GUARDRAILS = { "Format-Volume", "Set-ExecutionPolicy" ], - "maxCommandLength": 2000 + "maxCommandLength": 2000, + "blockedInterimTriggerEventTypes": [ + "performance_snapshot", + "agent_metrics_snapshot", + "download_progress", + "network_state_change", + "network_connectivity_check", + "log_entry", + "agent_trace", + "stall_probe_check" + ] } as const; diff --git a/src/McpServer/autopilot-monitor-mcp/src/rule-validation.ts b/src/McpServer/autopilot-monitor-mcp/src/rule-validation.ts index e8a68d01..137a2d37 100644 --- a/src/McpServer/autopilot-monitor-mcp/src/rule-validation.ts +++ b/src/McpServer/autopilot-monitor-mcp/src/rule-validation.ts @@ -387,10 +387,10 @@ function lintAnalyzeRule(rule: Record): ValidationFinding[] { if (trigger.startsWith('on_event:')) { const eventType = trigger.slice('on_event:'.length); findings.push(...checkEventTypeKnown(eventType, `evaluateOn "${trigger}"`)); - if (HIGH_FREQUENCY_EVENT_TYPES.has(eventType)) { + if (BLOCKED_INTERIM_TRIGGER_EVENT_TYPES.has(eventType)) { findings.push({ - level: 'warning', - message: `evaluateOn "${trigger}": ${eventType} is a high-frequency telemetry event — this would enqueue an interim analyze run on nearly every ingest batch. Pick a rare, problem-indicating event type instead.`, + level: 'error', + message: `evaluateOn "${trigger}": ${eventType} is a HARD-BLOCKED high-frequency telemetry event — it occurs on nearly every ingest batch and would run analysis continuously. The backend rejects this trigger on save and the runtime ignores it. Pick a rare, problem-indicating event type instead.`, }); } } @@ -430,18 +430,14 @@ function lintAnalyzeRule(rule: Record): ValidationFinding[] { /** * Event types emitted on effectively every batch (telemetry cadence) — an on_event interim - * trigger keyed to one of these would turn every ingest into an analyze run. + * trigger keyed to one of these would turn every ingest into an analyze run. Sourced from + * the baked guardrails mirror (rules/guardrails.json → RULE_GUARDRAILS); enforcement itself + * is code-only in the backend (AnalyzeRuleTriggers.BlockedOnEventTypes) with a parity test + * pinning the two sets. */ -const HIGH_FREQUENCY_EVENT_TYPES = new Set([ - 'performance_snapshot', - 'agent_metrics_snapshot', - 'download_progress', - 'network_state_change', - 'network_connectivity_check', - 'log_entry', - 'agent_trace', - 'stall_probe_check', -]); +const BLOCKED_INTERIM_TRIGGER_EVENT_TYPES = new Set( + ((RULE_GUARDRAILS as Record).blockedInterimTriggerEventTypes as readonly string[] | undefined) ?? [], +); // ── Gather-rule semantic lint ─────────────────────────────────────────────── diff --git a/src/Shared/AutopilotMonitor.Shared/Models/Rules/AnalyzeRuleTriggers.cs b/src/Shared/AutopilotMonitor.Shared/Models/Rules/AnalyzeRuleTriggers.cs index 15bc30ab..c751a583 100644 --- a/src/Shared/AutopilotMonitor.Shared/Models/Rules/AnalyzeRuleTriggers.cs +++ b/src/Shared/AutopilotMonitor.Shared/Models/Rules/AnalyzeRuleTriggers.cs @@ -22,6 +22,31 @@ namespace AutopilotMonitor.Shared.Models /// Prefix for event-driven interim triggers: on_event:<eventType>. public const string OnEventPrefix = "on_event:"; + /// + /// HARD-BLOCKED on_event trigger types: telemetry-cadence events that occur on + /// effectively every ingest batch — an interim trigger keyed to one of these would turn + /// every batch into an analyze run (full event-stream read per batch, per session). + /// Enforcement is code-only by design (this set gates CRUD validation AND the runtime + /// matching in , so a row that ever slips past validation is + /// still inert). rules/guardrails.json carries the display/pre-flight mirror consumed + /// by the MCP validate_rule lint; a parity test pins the two against drift. + /// + public static readonly IReadOnlyCollection BlockedOnEventTypes = new HashSet(StringComparer.OrdinalIgnoreCase) + { + "performance_snapshot", + "agent_metrics_snapshot", + "download_progress", + "network_state_change", + "network_connectivity_check", + "log_entry", + "agent_trace", + "stall_probe_check", + }; + + /// True when the event type is hard-blocked as an on_event interim trigger. + public static bool IsBlockedOnEventType(string? eventType) + => !string.IsNullOrWhiteSpace(eventType) && BlockedOnEventTypes.Contains(eventType!.Trim()); + /// /// The rule's effective trigger list: EvaluateOn, or the enrollment_end default when /// the field is absent/empty (legacy rules and rows predating the feature). @@ -44,6 +69,9 @@ namespace AutopilotMonitor.Shared.Models /// /// The event types this rule wants an interim run for (empty when it has no /// on_event triggers). Types are returned lower-cased for set intersection. + /// Hard-blocked types () are silently dropped — + /// defense in depth: even a persisted row carrying one (pre-validation data, direct + /// table write) can never cause an interim run. /// public static IReadOnlyList OnEventTypes(AnalyzeRule rule) { @@ -51,7 +79,7 @@ namespace AutopilotMonitor.Shared.Models .Where(t => t != null && t.StartsWith(OnEventPrefix, StringComparison.OrdinalIgnoreCase) && t.Length > OnEventPrefix.Length) .Select(t => t.Substring(OnEventPrefix.Length).Trim().ToLowerInvariant()) - .Where(t => t.Length > 0) + .Where(t => t.Length > 0 && !IsBlockedOnEventType(t)) .ToList(); }