From 81c14bb3edd8738ea68e0b941fe55bad388c7a97 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Fri, 21 Aug 2026 14:13:46 +0200 Subject: [PATCH] fix(rules): close the rule-ID namespace gaps around the shared global keyspace Custom rules are tenant-partitioned and functionally collision-free, but three surfaces still keyed on the bare non-unique ruleId string: - RuleStats global aggregate (global_{ruleId}) summed same-ID custom rules of unrelated tenants (title/severity last-writer-wins). The global row is now catalog-only: analyze gates on IsBuiltIn/IsCommunity, gather gates on the reserved-namespace pattern (agent events carry no flag; custom rules can never occupy the reserved pattern). - The 'never ship a built-in in the CUSTOM namespace' commitment from RuleIdPolicy was documented but unenforced: combine.js now fails on duplicate or non-reserved built-in IDs, BuiltInRuleCatalogPolicyTests pins the same invariants on the embedded catalog, and the GitHub reseed rejects non-reserved IDs before writing to the global partition. - DeleteRuleStatesForRuleIdAcrossTenantsAsync (cross-partition delete by bare RowKey) now refuses IDs outside the reserved namespace with failed=-1 so callers abort the follow-up global catalog delete. Verified: prod global partitions (45 analyze + 11 gather) are fully inside the reserved namespace, so the guards touch no existing data. New OKF doc docs/rules/rule-id-namespace.md captures the identity/collision model. Co-Authored-By: Claude Fable 5 --- docs/index.md | 1 + docs/log.md | 2 + docs/rules/rule-id-namespace.md | 47 ++++++++++++++++ rules/scripts/combine.js | 27 ++++++++- .../BuiltInRuleCatalogPolicyTests.cs | 56 +++++++++++++++++++ .../Admin/ReseedFromGitHubFunction.cs | 24 ++++++++ .../Analyze/AnalyzeOnEnrollmentEndHandler.cs | 14 +++-- .../EventIngestProcessor.RuleStats.cs | 16 ++++-- .../Services/TableStorageService.Rules.cs | 14 +++++ 9 files changed, 191 insertions(+), 10 deletions(-) create mode 100644 docs/rules/rule-id-namespace.md create mode 100644 src/Backend/AutopilotMonitor.Functions.Tests/BuiltInRuleCatalogPolicyTests.cs diff --git a/docs/index.md b/docs/index.md index 7bba8cd4..cef19ccb 100644 --- a/docs/index.md +++ b/docs/index.md @@ -37,6 +37,7 @@ https://docs.autopilotmonitor.com (separate repository). * [Gather Rule Phase Scoping, One-Shot Triggers & Emit-on-Change](rules/gather-rule-phase-scoping.md) - Restricting gather rules to enrollment phases (activePhases / activeFromPhase sticky latch), one-shot collection at a phase's start/end (phase_change / phase_exit), and emitMode "on_change" result-dedup. * [Gather Rule Guardrails](rules/gather-rule-guardrails.md) - Why the allowlists in rules/guardrails.json are enforced on the agent and nowhere else, which hard blocks survive unrestricted mode, and the rule that every collector must call a guard and emit security_warning on refusal. * [Rule Authoring Surface](rules/rule-authoring-surface.md) - How customer AIs author rules against the real contract: the baked MCP resources (guide/schemas/guardrails via combine.js), the MCP-local validate_rule pre-flight, and the side-effect-free POST rules/analyze/dryrun whose trace twin shares the production evaluators (parity + side-effect-freedom pinned by tests). +* [Rule ID Namespace & Collision Guards](rules/rule-id-namespace.md) - Why rule identity is the human-readable ruleId (no GUIDs) with tenant isolation via Table Storage partitioning, the reserved built-in namespace (RuleIdPolicy), which collisions are possible (custom-vs-custom across tenants: allowed and harmless), and the guards keeping the global RuleStats aggregate catalog-only and the global partition inside the reserved namespace (combine.js, reseed, cross-tenant RuleState GC). * [Analyze Rule Evaluation Triggers](rules/analyze-rule-triggers.md) - Opt-in `evaluateOn` interim triggers (`whiteglove_sealed`, `on_event:`) so rules can fire before a session is terminal, with RuleResults update-semantics (interim → finalize/resolve, NotifiedAt notification dedupe) replacing the permanent dedupe freeze, KO/stat suppression on interim runs, the ingest-side cached trigger registry, and the authoring lints that keep interim rules monotonic. # Backend diff --git a/docs/log.md b/docs/log.md index ba96895c..2fdc3b59 100644 --- a/docs/log.md +++ b/docs/log.md @@ -2,6 +2,8 @@ ## 2026-08-21 +* **New**: `rules/rule-id-namespace.md` — rule identity model: human-readable `ruleId` strings (no GUIDs), tenant isolation via Table Storage partitioning (`global` = built-in/community, tenantId = custom), the reserved built-in namespace from `RuleIdPolicy`, and the four collision guards: catalog-only global `RuleStats` aggregate (same-ID custom rules of unrelated tenants no longer sum into one `global_{ruleId}` row), combine.js duplicate/namespace fail, GitHub-reseed rejection of non-reserved IDs into the global partition, and the namespace gate on the cross-tenant RuleState GC. + * **New**: `backend/app-install-duration-attempt.md` — attempt-scoped app install durations: `DurationSeconds` = status-defining (last) attempt anchored on the new max-folded `LastAttemptStartedAt` (StartedAt stays the earliest-wins window/version-ordering key; legacy rows without the anchor keep the span fallback); `InstallPassCount` with replay-guarded additive fold; weaker-terminal guard (completed-with-Skipped never overrides Installed/Error, in-batch and cross-batch); attempt-scoped `DownloadDurationSeconds` pairing; the version-regression radar's both-sides cutover gate; time attribution counts the union of per-pass active segments (occupancy merges segments, hulls stay for display/what-if); agent payload timing re-arms both endpoints on terminal→active. No backfill — pre-cutover rows keep span semantics. * **Update**: `agent/decision-engine.md` — activity-based RealmJoin timeout extension (report 55e6afd61c9d: a large RJ package catalog outlived the 60-min window mid-install — the timer arms at RJ-agent-MSI detection, so the first deployment had effectively ~44 min). When the `RealmJoinTimeout` deadline fires with `LastDeploymentPhase` 100/101 and deployment activity (phase change or package observation, persisted in `RealmJoinFacts.LastActivityUtc`; the detection-time phase capture deliberately excluded) inside the last 60 min, the deadline re-arms to `lastActivity + 60 min` (Info event `realmjoin_timeout_extended`), clamped at detection + 4 h. `realmjoin_timeout` now carries `reason` = `hard_timeout` / `inactivity` / `absolute_cap`; a fire of a superseded deadline incarnation dead-ends. Idle detection and phase-200/210 sessions keep the original 60-min behavior. diff --git a/docs/rules/rule-id-namespace.md b/docs/rules/rule-id-namespace.md new file mode 100644 index 00000000..f9c17d72 --- /dev/null +++ b/docs/rules/rule-id-namespace.md @@ -0,0 +1,47 @@ +--- +type: concept +title: Rule ID Namespace & Collision Guards +description: How gather/analyze rule identity is scoped (tenant partition vs global partition), why there are no GUIDs, which collisions are possible, and the guards that keep the reserved built-in namespace and the global RuleStats aggregate clean. +resource: backend, rules +tags: [rules, ruleid, namespace, collision, rulestats, tenant-isolation] +timestamp: 2026-08-21 +--- + +# Schema + +Rule identity is the human-readable `ruleId` string — there are no GUIDs, deliberately: the ID appears in logs, mails, notifications, RuleResults, the agent config payload, and docs, and tenant isolation is provided by Table Storage partitioning instead. + +| Store | PartitionKey | RowKey | Content | +|---|---|---|---| +| `GatherRules` / `AnalyzeRules` | `"global"` | ruleId | Built-in + community rules (shared, all tenants) | +| `GatherRules` / `AnalyzeRules` | tenantId | ruleId | Custom rules (visible only to that tenant) | +| `RuleStates` | tenantId | ruleId | Per-tenant enable/notify overrides — built-in/community rules ONLY (custom rules fold state into their own row) | +| `RuleStats` | date | `{tenantId}_{ruleId}` and `global_{ruleId}` | Daily fire/eval telemetry; the global row is catalog-only (see guards) | + +Namespace split (`RuleIdPolicy.cs`, shared model): + +- **Reserved built-in namespace**: `(ANALYZE|GATHER)--` with `CATEGORY != CUSTOM`, case-insensitive, including currently unused numbers (gaps = retired rules that may return). +- **Tenant namespace**: everything else — `ANALYZE-CUSTOM-NNN` (portal suggestion), `-CUSTOM` suffixes, org prefixes (`CONTOSO-WIFI-001`), freeform. + +# Collision model + +- **Custom vs built-in**: impossible via API — `CreateRuleAsync`/`UpdateRuleAsync` reject reserved IDs (409) and check existence against `global` ∪ own tenant partition. Legacy debris is neutralized at merge time: global wins, tenant copy dropped with a warning (pinned by `RuleMergeCollisionGuardTests`, motivated by the 5ca2b350 duplicate-key incident). +- **Custom vs custom across tenants**: allowed and harmless — separate partitions, separate evaluation, separate RuleResults/tenant stats. Two tenants both using `ANALYZE-CUSTOM-101` never interact. +- **JSON copy between tenants** (export → paste into create editor): goes through the same create path, so an ID already taken in the receiving tenant surfaces as 409 — the user renames. There is no bulk import endpoint; note the export→paste round-trip is lossy (gather `parameters`, analyze `evaluateOn` are form-derived and silently dropped by the create editor). + +# Guards (all enforced, 2026-08-21) + +1. **Global RuleStats aggregate is catalog-only.** The `global_{ruleId}` row would otherwise sum same-ID custom rules of unrelated tenants (title/severity last-writer-wins). Analyze gates on `rule.IsBuiltIn || rule.IsCommunity` (`AnalyzeOnEnrollmentEndHandler`), gather gates on `RuleIdPolicy.IsReservedBuiltInId` (`EventIngestProcessor.RuleStats`) because agent events carry no flag — valid because custom rules can never occupy the reserved pattern. +2. **Catalog build fails on namespace violations.** `rules/scripts/combine.js` rejects duplicate IDs (case-insensitive) and any gather/analyze built-in outside the reserved pattern; `BuiltInRuleCatalogPolicyTests` pins the same invariants on the embedded resource actually deployed. +3. **GitHub reseed rejects non-reserved IDs** before writing to the `global` partition (`ReseedFromGitHubFunction`) — a seeded `*-CUSTOM-*` rule would silently shadow every tenant's same-ID custom rule at merge time. +4. **Cross-tenant RuleState GC is namespace-gated.** `DeleteRuleStatesForRuleIdAcrossTenantsAsync` deletes by bare RowKey across all partitions; it now refuses non-reserved IDs with `failed=-1` (callers then skip the global catalog delete, same contract as enumeration failure). + +Invariant chain: schema pattern + combine.js + reseed guard ⇒ the `global` partition only ever contains reserved-pattern IDs ⇒ the pattern is a safe built-in/custom discriminator everywhere no `IsBuiltIn` flag is available. + +# Citations + +- `src/Shared/AutopilotMonitor.Shared/Models/Rules/RuleIdPolicy.cs` — reserved pattern + rationale +- `src/Backend/AutopilotMonitor.Functions/Services/AnalyzeRuleService.cs`, `GatherRuleService.cs` — create/update guards, merge collision guard +- `src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Rules.cs` — key schemes, cross-tenant GC guard +- `src/Backend/AutopilotMonitor.Functions.Tests/BuiltInRuleCatalogPolicyTests.cs`, `RuleIdReservationTests.cs`, `RuleMergeCollisionGuardTests.cs` +- `rules/scripts/combine.js` — build-time namespace/duplicate guard diff --git a/rules/scripts/combine.js b/rules/scripts/combine.js index 85c44b15..4fad96db 100644 --- a/rules/scripts/combine.js +++ b/rules/scripts/combine.js @@ -13,18 +13,26 @@ const rulesRoot = path.resolve(__dirname, '..'); // ── Rule combination ──────────────────────────────────────────────────────── +// Shipped gather/analyze rules must stay inside the reserved built-in namespace: +// (ANALYZE|GATHER)-- with CATEGORY !== CUSTOM. The CUSTOM category is +// the sanctioned tenant namespace (RuleIdPolicy.cs) — a built-in shipped there would +// silently shadow every tenant's same-ID custom rule at merge time (global wins). +const RESERVED_BUILTIN_ID = /^(ANALYZE|GATHER)-(?!CUSTOM-)[A-Z]+-\d{3}$/; + const configs = [ { dir: path.join(rulesRoot, 'gather'), output: path.join(rulesRoot, 'dist', 'gather-rules.json'), schema: '../schema/gather-rule.schema.json', - idField: 'ruleId' + idField: 'ruleId', + reservedNamespace: RESERVED_BUILTIN_ID }, { dir: path.join(rulesRoot, 'analyze'), output: path.join(rulesRoot, 'dist', 'analyze-rules.json'), schema: '../schema/analyze-rule.schema.json', - idField: 'ruleId' + idField: 'ruleId', + reservedNamespace: RESERVED_BUILTIN_ID }, { dir: path.join(rulesRoot, 'ime-log-patterns'), @@ -37,12 +45,27 @@ const configs = [ for (const config of configs) { const files = fs.readdirSync(config.dir).filter(f => f.endsWith('.json')).sort(); const rules = []; + const seenIds = new Map(); // lowercased id -> filename (case variants collide for humans and RuleIdPolicy) for (const file of files) { const content = fs.readFileSync(path.join(config.dir, file), 'utf8'); const rule = JSON.parse(content); // Remove $schema from individual entries (it's on the wrapper) delete rule['$schema']; + + const id = rule[config.idField] || ''; + const idKey = id.toLowerCase(); + if (seenIds.has(idKey)) { + console.error(`ERROR: duplicate ${config.idField} "${id}" in ${file} (already defined in ${seenIds.get(idKey)})`); + process.exit(1); + } + seenIds.set(idKey, file); + + if (config.reservedNamespace && !config.reservedNamespace.test(id)) { + console.error(`ERROR: ${file}: ${config.idField} "${id}" is outside the reserved built-in namespace ${config.reservedNamespace} — built-ins must never use the tenant CUSTOM namespace`); + process.exit(1); + } + rules.push(rule); } diff --git a/src/Backend/AutopilotMonitor.Functions.Tests/BuiltInRuleCatalogPolicyTests.cs b/src/Backend/AutopilotMonitor.Functions.Tests/BuiltInRuleCatalogPolicyTests.cs new file mode 100644 index 00000000..90ea36a3 --- /dev/null +++ b/src/Backend/AutopilotMonitor.Functions.Tests/BuiltInRuleCatalogPolicyTests.cs @@ -0,0 +1,56 @@ +using AutopilotMonitor.Functions.Services; +using AutopilotMonitor.Shared.Models; + +namespace AutopilotMonitor.Functions.Tests; + +/// +/// Pins the shipped rule catalogs to the reserved built-in ID namespace from +/// RuleIdPolicy: every built-in occupies (ANALYZE|GATHER)-<CATEGORY>-<NUMBER> with +/// CATEGORY != CUSTOM, and IDs are unique (case-insensitively — case variants are +/// indistinguishable to humans and to the policy regex). This is the backend-side +/// enforcement of the "platform never ships a built-in in the CUSTOM namespace" +/// commitment documented in RuleIdPolicy — without it, a shipped CUSTOM-ID rule +/// would silently shadow every tenant's same-ID custom rule at merge time +/// (global wins, tenant copy dropped). combine.js mirrors these checks at +/// catalog-build time; this test guards the embedded resource actually deployed. +/// +public class BuiltInRuleCatalogPolicyTests +{ + [Fact] + public void BuiltIn_analyze_ids_are_reserved_namespace_and_unique() + { + var ids = BuiltInAnalyzeRules.GetAll().Select(r => r.RuleId).ToList(); + + Assert.NotEmpty(ids); + foreach (var id in ids) + { + Assert.True(RuleIdPolicy.IsReservedBuiltInId(id), + $"Built-in analyze rule '{id}' lies outside the reserved built-in namespace — " + + "it would collide with the tenant custom namespace."); + } + + var duplicates = ids.GroupBy(id => id, StringComparer.OrdinalIgnoreCase) + .Where(g => g.Count() > 1).Select(g => g.Key).ToList(); + Assert.True(duplicates.Count == 0, + $"Duplicate built-in analyze rule IDs: {string.Join(", ", duplicates)}"); + } + + [Fact] + public void BuiltIn_gather_ids_are_reserved_namespace_and_unique() + { + var ids = BuiltInGatherRules.GetAll().Select(r => r.RuleId).ToList(); + + Assert.NotEmpty(ids); + foreach (var id in ids) + { + Assert.True(RuleIdPolicy.IsReservedBuiltInId(id), + $"Built-in gather rule '{id}' lies outside the reserved built-in namespace — " + + "it would collide with the tenant custom namespace."); + } + + var duplicates = ids.GroupBy(id => id, StringComparer.OrdinalIgnoreCase) + .Where(g => g.Count() > 1).Select(g => g.Key).ToList(); + Assert.True(duplicates.Count == 0, + $"Duplicate built-in gather rule IDs: {string.Join(", ", duplicates)}"); + } +} diff --git a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/ReseedFromGitHubFunction.cs b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/ReseedFromGitHubFunction.cs index 4057c74a..13f9e35d 100644 --- a/src/Backend/AutopilotMonitor.Functions/Functions/Admin/ReseedFromGitHubFunction.cs +++ b/src/Backend/AutopilotMonitor.Functions/Functions/Admin/ReseedFromGitHubFunction.cs @@ -137,6 +137,18 @@ namespace AutopilotMonitor.Functions.Functions.Admin private async Task<(int deleted, int written, int orphanStatesGcd, int sunsetSkipped)> ReseedGatherAsync(List rules) { + // Global-partition rules must stay inside the reserved built-in namespace — an ID + // outside it (e.g. GATHER-CUSTOM-*) would silently shadow every tenant's same-ID + // custom rule at merge time (global wins). Reject instead of import. + var rejectedGather = rules.Where(r => !AutopilotMonitor.Shared.Models.RuleIdPolicy.IsReservedBuiltInId(r.RuleId)).ToList(); + if (rejectedGather.Count > 0) + { + _logger.LogError( + "GitHub reseed gather: rejected {Count} rule(s) outside the reserved built-in namespace: {Ids}", + rejectedGather.Count, string.Join(", ", rejectedGather.Select(r => r.RuleId))); + rules = rules.Except(rejectedGather).ToList(); + } + var existing = await _ruleRepo.GetGatherRulesAsync("global"); var newCatalogIds = rules.Select(r => r.RuleId).ToHashSet(System.StringComparer.Ordinal); // A rule the deployed binary already ships IDENTICALLY is embedded-provenance; one whose @@ -199,6 +211,18 @@ namespace AutopilotMonitor.Functions.Functions.Admin private async Task<(int deleted, int written, int orphanStatesGcd, int sunsetSkipped)> ReseedAnalyzeAsync(List rules) { + // Global-partition rules must stay inside the reserved built-in namespace — an ID + // outside it (e.g. ANALYZE-CUSTOM-*) would silently shadow every tenant's same-ID + // custom rule at merge time (global wins). Reject instead of import. + var rejectedAnalyze = rules.Where(r => !AutopilotMonitor.Shared.Models.RuleIdPolicy.IsReservedBuiltInId(r.RuleId)).ToList(); + if (rejectedAnalyze.Count > 0) + { + _logger.LogError( + "GitHub reseed analyze: rejected {Count} rule(s) outside the reserved built-in namespace: {Ids}", + rejectedAnalyze.Count, string.Join(", ", rejectedAnalyze.Select(r => r.RuleId))); + rules = rules.Except(rejectedAnalyze).ToList(); + } + var existing = await _ruleRepo.GetAnalyzeRulesAsync("global"); var newCatalogIds = rules.Select(r => r.RuleId).ToHashSet(System.StringComparer.Ordinal); // A rule the deployed binary already ships IDENTICALLY is embedded-provenance; one whose diff --git a/src/Backend/AutopilotMonitor.Functions/Services/Analyze/AnalyzeOnEnrollmentEndHandler.cs b/src/Backend/AutopilotMonitor.Functions/Services/Analyze/AnalyzeOnEnrollmentEndHandler.cs index 02d201c5..719c4269 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/Analyze/AnalyzeOnEnrollmentEndHandler.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/Analyze/AnalyzeOnEnrollmentEndHandler.cs @@ -289,10 +289,16 @@ namespace AutopilotMonitor.Functions.Services.Analyze rule.Title, rule.Category, rule.Severity, fired, confidence).ConfigureAwait(false); - await _metricsRepo.IncrementRuleStatAsync( - today, "global", rule.RuleId, "analyze", - rule.Title, rule.Category, rule.Severity, - fired, confidence).ConfigureAwait(false); + // Global aggregate row is catalog-only: custom-rule IDs are tenant-chosen + // and not unique across tenants, so a shared "global_{ruleId}" row would + // sum unrelated tenants' counters (title/severity last-writer-wins). + if (rule.IsBuiltIn || rule.IsCommunity) + { + await _metricsRepo.IncrementRuleStatAsync( + today, "global", rule.RuleId, "analyze", + rule.Title, rule.Category, rule.Severity, + fired, confidence).ConfigureAwait(false); + } } } catch (Exception ex) diff --git a/src/Backend/AutopilotMonitor.Functions/Services/EventIngestProcessor.RuleStats.cs b/src/Backend/AutopilotMonitor.Functions/Services/EventIngestProcessor.RuleStats.cs index 878e47bd..1aaf4d0c 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/EventIngestProcessor.RuleStats.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/EventIngestProcessor.RuleStats.cs @@ -36,10 +36,18 @@ namespace AutopilotMonitor.Functions.Services ruleTitle, "", "", fired: true, confidenceScore: null); - await _metricsRepo.IncrementRuleStatAsync( - today, "global", ruleId, "gather", - ruleTitle, "", "", - fired: true, confidenceScore: null); + // Global aggregate row is catalog-only. The agent event carries no + // IsBuiltIn flag, but custom rules can never occupy the reserved + // built-in namespace (create/update guard), so the ID pattern is the + // discriminator. Custom IDs are tenant-chosen and not unique across + // tenants — a shared "global_{ruleId}" row would mix their counters. + if (RuleIdPolicy.IsReservedBuiltInId(ruleId)) + { + await _metricsRepo.IncrementRuleStatAsync( + today, "global", ruleId, "gather", + ruleTitle, "", "", + fired: true, confidenceScore: null); + } } } catch (Exception ex) diff --git a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Rules.cs b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Rules.cs index 572b4fad..6c807cde 100644 --- a/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Rules.cs +++ b/src/Backend/AutopilotMonitor.Functions/Services/TableStorageService.Rules.cs @@ -442,6 +442,20 @@ namespace AutopilotMonitor.Functions.Services public async Task<(int deleted, int failed)> DeleteRuleStatesForRuleIdAcrossTenantsAsync(string ruleId) { if (string.IsNullOrWhiteSpace(ruleId)) return (0, 0); + + // This is a cross-partition delete keyed on a bare RowKey string, so only + // catalog (built-in/community) IDs may pass: custom-rule IDs are tenant-chosen + // and not unique across tenants, and a call with one would delete state rows + // of unrelated tenants. failed=-1 reuses the enumeration-failure contract — + // the caller then refuses the follow-up global catalog delete. + if (!RuleIdPolicy.IsReservedBuiltInId(ruleId)) + { + _logger.LogError( + "Refusing cross-tenant RuleState GC for '{RuleId}': ID is outside the reserved built-in namespace", + ruleId); + return (0, -1); + } + try { var tableClient = _tableServiceClient.GetTableClient(Constants.TableNames.RuleStates);