From 7aa37022efaad61ba2b7ab8545065ceb03366d8a Mon Sep 17 00:00:00 2001 From: Guilherme Cota <1190640@isep.ipp.pt> Date: Mon, 19 Oct 2026 11:43:12 +0100 Subject: [PATCH] feat(code-reviewer)-process rule validation --- cmf-codereview/docs/architecture.md | 13 + cmf-codereview/docs/commands.md | 13 + cmf-codereview/docs/configuration.md | 18 + cmf-codereview/docs/rules.md | 60 ++- .../Analysis/ProcessRuleRepositoryContext.cs | 214 ++++++++++ cmf-codereview/src/CmFrameworkScanner.cs | 13 +- cmf-codereview/src/CodeReviewPaths.cs | 24 +- cmf-codereview/src/Models/ProcessRuleInfo.cs | 5 + cmf-codereview/src/Rules/ProcessRuleSyntax.cs | 321 +++++++++++++++ .../src/Rules/ProcessRuleValidationRule.cs | 268 +++++++++++++ cmf-codereview/src/Rules/RuleFileContext.cs | 14 +- .../tests/ProcessRuleIntegrationTests.cs | 210 ++++++++++ .../tests/ProcessRuleValidationTests.cs | 369 ++++++++++++++++++ 13 files changed, 1531 insertions(+), 11 deletions(-) create mode 100644 cmf-codereview/src/Analysis/ProcessRuleRepositoryContext.cs create mode 100644 cmf-codereview/src/Models/ProcessRuleInfo.cs create mode 100644 cmf-codereview/src/Rules/ProcessRuleSyntax.cs create mode 100644 cmf-codereview/src/Rules/ProcessRuleValidationRule.cs create mode 100644 cmf-codereview/tests/ProcessRuleIntegrationTests.cs create mode 100644 cmf-codereview/tests/ProcessRuleValidationTests.cs diff --git a/cmf-codereview/docs/architecture.md b/cmf-codereview/docs/architecture.md index bbfd3b8..aed0ce1 100644 --- a/cmf-codereview/docs/architecture.md +++ b/cmf-codereview/docs/architecture.md @@ -234,3 +234,16 @@ loop/method-analyzed at most once no matter how many passes touch it. **Adding a rule** doesn't touch any of this. It's one more class implementing `IReviewRule` (or one more instantiation of `LoopSensitiveCallRule` / `EntityPropertyAccessRule` if the shape already fits), registered in `CmFrameworkScanner.Rules`. See [docs/rules.md](rules.md). + +## Process Rule deployment context + +`ProcessRuleValidationRule` is registered with the normal built-in rules. After warming the +shared syntax cache, the scanner builds one read-only `ProcessRuleRepositoryContext`: package +content/phase/version classification, same-package MasterData schema changes, and resolvable +entity-type creator/consumer ordering. `RuleFileContext` carries this context into the parallel +rule loop; Process Rule detection reuses the cached Roslyn trees and yields ordinary `RuleMatch` +values. Existing enrichment, configuration, suppression, baseline, and changed-file filters apply. + +Deployment metadata comes from repository source/package files. The Markdown authoring guidelines +are implementation reference material and have no runtime role. No language model or policy +interpreter is involved. Findings use the existing JSON, Markdown, and HTML outputs. diff --git a/cmf-codereview/docs/commands.md b/cmf-codereview/docs/commands.md index 7389e59..277d171 100644 --- a/cmf-codereview/docs/commands.md +++ b/cmf-codereview/docs/commands.md @@ -192,6 +192,19 @@ Scan and produce a Detailed, self-contained HTML report for browser triage: cmf-codereview review --repository-root C:\repos\YourProject --report-mode Detailed --report-format Html ``` +## Reviewing Process Rules + +Process Rule diagnostics run automatically as part of the DEEs/Both scope. No guideline file +or additional validation flag is required. To focus on the Micron deployment rules: + +```powershell +cmf-codereview scan --repository-root C:\workspaces\Micron --roots Cmf.Custom.Data/Cmf.Custom.Data.Packages/DEEs/ProcessRules --fail-on High --output findings.json +cmf-codereview report --findings-path findings.json --report-mode Detailed --report-format Html --report-path process-rules.html +``` + +Supported Process Rule checks and deployment evidence that still needs review are documented in +[rules.md](rules.md#process-rule-deployment-validation). + ## Choosing between the three commands - Use **`review`** for everyday use — it's the one-command path from repository to report. diff --git a/cmf-codereview/docs/configuration.md b/cmf-codereview/docs/configuration.md index e5c5dc6..8bb1547 100644 --- a/cmf-codereview/docs/configuration.md +++ b/cmf-codereview/docs/configuration.md @@ -57,6 +57,24 @@ Notes: - Comments and trailing commas are allowed in the file. - A malformed file fails the scan with a clear error rather than being silently ignored. +## Process Rule checks + +Process Rule validation uses the same `rules` dictionary as every other built-in check: + +```json +{ + "rules": { + "ProcessRuleCondition": { "severity": "Critical" }, + "ProcessRuleUnresolvedGuard": { "enabled": false } + } +} +``` + +There is no `guidelinesPath` or policy-file configuration. The Markdown authoring guidelines +inform the C# implementation only; they are not loaded during scans. See +[rules.md](rules.md#process-rule-deployment-validation) for diagnostic IDs and supported patterns. +Each diagnostic can also be suppressed inline or accepted in the normal baseline. + ## Inline suppression comments ```csharp diff --git a/cmf-codereview/docs/rules.md b/cmf-codereview/docs/rules.md index e5fcf5c..dee86ba 100644 --- a/cmf-codereview/docs/rules.md +++ b/cmf-codereview/docs/rules.md @@ -1,6 +1,6 @@ # Rule Reference -All 17 rules implemented by `CmFrameworkScanner.GetFindings`. When a finding's severity escalates, the +Built-in checks implemented by `CmFrameworkScanner.GetFindings`. When a finding's severity escalates, the finding's JSON also carries a machine-readable `Escalation` object (base severity, reasons, nested-loop depth) — see [commands.md](commands.md#scan). Severity can escalate above the base level listed here for `LoadInsideLoop`, `ResolveInsideLoop`, `QueryExecuteInsideLoop`, `SaveInsideLoop`, @@ -67,6 +67,64 @@ ad-hoc query pattern). See `EntityCatalog.NonLazyLoadedReferenceNames` / | `DirectEntityInstantiation` | Medium | `new Material(`, `new Product(`, etc. — direct construction of a known entity type | Use `entityFactory.Create()` instead of direct construction. | | `GetConfigInsteadOfTryGetConfig` | Medium | A `GetConfig(` call that is not `TryGetConfig` and not `Config.GetConfig` (already covered by that rule) | Confirm whether this should use `TryGetConfig` or an existing wrapper service before keeping the direct call. | +## Process Rule deployment validation + +`ProcessRuleValidationRule` implements deterministic Roslyn checks for `DeeDevBase` deployment +classes. The authoring guidelines are development reference material: **the scanner never reads +`process-rule-validation-guidelines.md`**. Identical source, package metadata, tool version, and +review configuration produce identical findings. Changes to checks are C# changes with tests. + +Process Rules are discovered from Data-package `EntityTypes`, `ProcessRulesPre`, and +`ProcessRulesPost` content entries, including repositories without a general `DEE` entry. +Standard `DEEs/ProcessRules/` paths also work with explicit roots. Classification distinguishes +versioned Before/After, EntityTypes, EntityTypesAttributes, and recurring unversioned Services. +The older `IsOneTimeMigrationFile` exemptions remain unchanged for compatibility; that flag alone +does not imply a Services rule runs once. + +| Diagnostic | Default severity | Detects / remediation | +|---|---|---| +| `ProcessRuleCondition` | High | Condition contains more than an unconditional `return true` / `=> true`. Deployment skips it; move guards into `DeeActionCode`. | +| `ProcessRuleUnguardedMutation` | High; Critical when the same guard exists only in the skipped condition | Attribute creation or typed Action/Rule/Config/EntityType creation, deletion, removal, saving, action-group removal, or termination lacks a recognized guard for its own target. | +| `ProcessRuleUnresolvedGuard` | Medium | Attribute target or a helper-based guard cannot be resolved. Review the helper rather than treating it as proven safe. | +| `ProcessRuleTerminationState` | High | Termination lacks an already-terminated state guard. Check both existence and `UniversalState != Terminated`. | +| `ProcessRuleSchemaRegeneration` | High | Raw property collection creation or `FullUpdateEntityType` property changes lack subsequent schema regeneration for the same entity on the same path. | +| `ProcessRuleAttributeRemoval` | High; Critical when matching cleanup occurs after removal | A resolved property removal has no matching earlier `RemoveAttributes` cleanup. Clear all holders and preserve values where required before removing metadata. | +| `ProcessRuleAttributeRemovalReview` | Medium | Removed properties are supplied through an unresolved collection/helper; verify guards and cleanup. | +| `ProcessRuleTableMigration` | Critical | Smart/Generic Table data access or mutation targets schema created/changed in the same package version's MasterData. Move migration to a post-install step or later independent package. | +| `ProcessRuleTableMigrationReview` | Medium | Table mutation has an unresolved name/package/version; verify deployment dependencies and rerun safety. | +| `ProcessRulePlacement` | High | Unsupported folder, conflicting namespace phase/content type, wrong position relative to MasterData, or attribute changes in a general Before/After folder. | +| `ProcessRuleVersionNamespace` | Medium | Namespace version does not match the dotted folder version's underscore form. | +| `ProcessRuleRecurringMigration` | Medium | Recognizable migration mutations under Services will run on every deployment; confirm intent or move to a versioned folder. | +| `ProcessRuleDependencyOrder` | High | A literal entity-type load precedes its unique same-package, same-version creation rule in manifest/alphabetical file order. Move the creator earlier. | + +Guards recognize `HasProperty`, `TryGetEntityTypeProperty`, `ObjectExists`, `TryLoad`, boolean +negation/comparisons, applicable branches, and early returns. Checks on another property or target +do not protect an operation. Reachable local functions are analyzed, including guards at their +call sites; dormant local functions and callbacks are excluded. Raw metadata changes recognize +`GenerateSchema` and `GenerateEntityTypeDBSchema` after the update. The normal +`IMicronEntityTypeService.UpdateEntityTypeProperties` route does not require a duplicate schema +call. A guarded removal followed by recreation supports deliberate attribute-definition repairs; +the removal still needs value cleanup. + +Schema context is read once per scan from the same Data package's versioned MasterData. It tracks +new Smart/Generic Tables and changes to property structure. Row data, precedence keys, and +description-only updates are not schema changes. Partial property updates retain earlier field +definitions. Names must resolve from local/constant strings, a table's `Name`, or a named `Load`; +unrelated packages' tables are never correlated. Unchanged files provide context even when +`--changed-files-list` restricts the reported findings. + +These are bounded syntax checks, not execution or whole-program proofs. External helpers, +callbacks, indirect SQL/query table dependencies, and arbitrary alias/data flow may require +review. Cleanup recognition does not prove a query included every populated holder. Rename/move +value preservation is a developer decision; verify copies finish before cleanup. Released-version +immutability, database execution twice, complete test coverage, and manual release-note steps +need release/deployment evidence and are not inferred from folder age or missing static findings. +Do not edit shipped rules to resolve historical findings: fix forward, or baseline accepted debt. + +All diagnostics use the existing per-rule settings, inline suppressions, baselines, changed-file +filtering, severity gates, and JSON/Markdown/HTML reporting. They do not use loop escalation. + + ## The entity catalog (`src/Rules/EntityCatalog.cs`) `SaveInsideLoop`, `CreateInstanceInsideLoop`, `DeleteRemoveInsideLoop`, `ExcessiveLazyLoading`, diff --git a/cmf-codereview/src/Analysis/ProcessRuleRepositoryContext.cs b/cmf-codereview/src/Analysis/ProcessRuleRepositoryContext.cs new file mode 100644 index 0000000..81ff3a6 --- /dev/null +++ b/cmf-codereview/src/Analysis/ProcessRuleRepositoryContext.cs @@ -0,0 +1,214 @@ +using System.Text.Json; +using System.Text.RegularExpressions; +using Cmf.Cli.Plugin.CodeReview.Models; +using Cmf.Cli.Plugin.CodeReview.Rules; +using Microsoft.CodeAnalysis.CSharp.Syntax; + +namespace Cmf.Cli.Plugin.CodeReview; + +/// Read-only deployment metadata shared by all rule contexts in one scan. No Markdown inputs. +public sealed class ProcessRuleRepositoryContext +{ + private readonly Dictionary _files = new(StringComparer.OrdinalIgnoreCase); + private readonly Dictionary<(string Package, string Version, string Table), string> _schemaChanges = new(); + private readonly Dictionary> _orderProblems = new(StringComparer.OrdinalIgnoreCase); + private static readonly Regex VersionSegment = new(@"^\d+\.\d+\.\d+(?:\.\d+)?$", RegexOptions.Compiled); + + public ProcessRuleInfo? GetInfo(string file) => _files.GetValueOrDefault(Path.GetFullPath(file)); + public IReadOnlyList GetOrderProblems(string file) => _orderProblems.GetValueOrDefault(Path.GetFullPath(file)) ?? []; + + public string? GetSchemaChange(ProcessRuleInfo info, string table) => + info.PackageDirectory is not null && info.Version is not null + ? _schemaChanges.GetValueOrDefault((info.PackageDirectory, info.Version, table)) : null; + + /// Fallback for explicit roots and isolated syntax-rule tests. + public static ProcessRuleInfo? ClassifyPath(string file) + { + var segments = file.Replace('\\', '/').Split('/'); + var start = Array.FindIndex(segments, s => s.Equals("ProcessRules", StringComparison.OrdinalIgnoreCase)); + if (start < 1 || !segments[start - 1].Equals("DEEs", StringComparison.OrdinalIgnoreCase)) return null; + var tail = segments.Skip(start + 1).ToArray(); + if (tail.Length == 2 && tail[0].Equals("Services", StringComparison.OrdinalIgnoreCase)) + return new(null, "Services"); + if (tail.Length == 3 && VersionSegment.IsMatch(tail[0]) && tail[1] is "Before" or "After") + return new(tail[0], tail[1]); + if (tail.Length == 3 && tail[0] is "EntityTypes" or "EntityTypesAttributes" && VersionSegment.IsMatch(tail[1])) + return new(tail[1], tail[0]); + return new(null, "Unknown"); + } + + public static ProcessRuleRepositoryContext Build(string repositoryRoot, IEnumerable files, CodeReviewFileCache? fileCache = null) + { + var context = new ProcessRuleRepositoryContext(); + var content = CodeReviewPaths.DiscoverDataPackageContent(repositoryRoot).ToList(); + var processEntries = content.Where(c => CodeReviewPaths.IsProcessRuleContent(c.ContentType)) + .Select(c => (Content: c, Pattern: SourcePattern(c.Source))).ToList(); + foreach (var file in files) + { + var fullFile = Path.GetFullPath(file); + foreach (var (entry, pattern) in processEntries) + { + var relative = Path.GetRelativePath(entry.Directory, fullFile).Replace('\\', '/'); + var match = pattern.Match(relative); + if (!match.Success) continue; + var fallback = ClassifyPath(fullFile); + var version = match.Groups["version"].Success ? match.Groups["version"].Value : fallback?.Version; + var phase = entry.ContentType.Equals("EntityTypes", StringComparison.OrdinalIgnoreCase) ? "EntityTypes" + : entry.ContentType.Equals("ProcessRulesPre", StringComparison.OrdinalIgnoreCase) ? "Before" + : relative.Split('/').Contains("EntityTypesAttributes") ? "EntityTypesAttributes" + : relative.Split('/').Contains("Services") ? "Services" : "After"; + var masterDataOrders = content.Where(c => c.Directory == entry.Directory && c.ContentType.Equals("MasterData", StringComparison.OrdinalIgnoreCase)) + .Select(c => c.Order).ToList(); + string? problem = fallback is { Phase: not "Unknown" } && fallback.Phase != phase + ? $"Folder phase {fallback.Phase} is packaged as {entry.ContentType} ({phase})." : null; + if (masterDataOrders.Count > 0 && (phase is "Before" or "EntityTypes" && entry.Order > masterDataOrders.Min() || + phase is "After" or "EntityTypesAttributes" or "Services" && entry.Order < masterDataOrders.Max())) + problem = $"{phase} content is on the wrong side of MasterData in cmfpackage.json contentToPack ordering."; + context._files[fullFile] = new(version, fallback is { Phase: not "Unknown" } ? fallback.Phase : phase, + entry.Directory, entry.ContentType, entry.Order, problem); + break; + } + if (!context._files.ContainsKey(fullFile) && ClassifyPath(fullFile) is { } info) + { + // An unlisted standard path still gets syntax validation, with no fabricated package facts. + var package = content.Where(c => fullFile.StartsWith(c.Directory + Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase)) + .OrderByDescending(c => c.Directory.Length).FirstOrDefault()?.Directory; + context._files[fullFile] = info with { PackageDirectory = package }; + } + } + if (context._files.Count != 0) + { + context.IndexSchemaChanges(repositoryRoot, content); + context.IndexDependencies(repositoryRoot, fileCache); + } + return context; + } + + private void IndexDependencies(string repositoryRoot, CodeReviewFileCache? fileCache) + { + var creators = new List<(string File, ProcessRuleInfo Info, string Entity)>(); + var consumers = new List<(string File, ProcessRuleInfo Info, string Entity)>(); + foreach (var (file, info) in _files.OrderBy(p => p.Key, StringComparer.Ordinal)) + { + if (info.Version is null || info.PackageDirectory is null) continue; + var root = fileCache?.Get(file).Root ?? CodeReviewSyntaxTree.Parse(File.ReadAllText(file)).Root; + foreach (var @class in root.DescendantNodes().OfType() + .Where(c => c.BaseList?.Types.Any(t => CodeReviewSyntaxTree.LastIdentifierSegment(t.Type.ToString()) == "DeeDevBase") == true)) + { + var action = @class.Members.OfType().FirstOrDefault(m => m.Identifier.ValueText == "DeeActionCode"); + if (action is null) continue; + var syntax = new ProcessRuleSyntax(@class, action); + foreach (var call in syntax.Calls) + { + var receiver = ProcessRuleSyntax.Receiver(call); + var isEntityType = syntax.TypeOf(receiver) is "IEntityType" or "EntityType"; + var name = ProcessRuleSyntax.Name(call); + if (name == "Create" && isEntityType && syntax.AssignedValue(ProcessRuleSyntax.Tokens(receiver), "Name", call) is { } assigned && + syntax.StringValue(assigned) is { } created) creators.Add((file, info, created)); + if ((name == "LoadEntityType" || name == "Load" && isEntityType) && call.ArgumentList.Arguments.FirstOrDefault()?.Expression is { } value && + syntax.StringValue(value) is { } loaded) consumers.Add((file, info, loaded)); + } + } + } + foreach (var consumer in consumers.Distinct()) + { + var matching = creators.Where(c => c.File != consumer.File && c.Info.PackageDirectory == consumer.Info.PackageDirectory && + c.Info.Version == consumer.Info.Version && c.Entity == consumer.Entity).ToList(); + // Multiple creators are ambiguous. Do not invent a dependency edge to one of them. + if (matching.Count != 1) continue; + var creator = matching[0]; + var creatorOrder = creator.Info.ContentOrder >= 0 ? creator.Info.ContentOrder : PhaseOrder(creator.Info.Phase); + var consumerOrder = consumer.Info.ContentOrder >= 0 ? consumer.Info.ContentOrder : PhaseOrder(consumer.Info.Phase); + if (creatorOrder < consumerOrder || creatorOrder == consumerOrder && + string.Compare(Path.GetFileName(creator.File), Path.GetFileName(consumer.File), StringComparison.Ordinal) < 0) continue; + if (!_orderProblems.TryGetValue(consumer.File, out var problems)) _orderProblems[consumer.File] = problems = []; + problems.Add($"Entity type '{consumer.Entity}' is loaded before its same-version creation rule '{CodeReviewPaths.GetRelativePath(repositoryRoot, creator.File)}' in declared content/alphabetical file order."); + } + } + + private static int PhaseOrder(string phase) => phase switch + { + "EntityTypes" => 0, "Before" => 1, "After" => 3, "EntityTypesAttributes" => 4, "Services" => 5, _ => 6 + }; + + private static Regex SourcePattern(string source) + { + var normalized = source.Replace('\\', '/'); + if (normalized.StartsWith("./", StringComparison.Ordinal)) normalized = normalized[2..]; + var pattern = Regex.Escape(normalized) + .Replace(Regex.Escape("$(version)"), @"(?\d+\.\d+\.\d+(?:\.\d+)?)") + .Replace(@"\*", ".*").Replace(@"\?", "."); + return new Regex("^" + pattern + "$", RegexOptions.CultureInvariant | RegexOptions.IgnoreCase); + } + + private void IndexSchemaChanges(string repositoryRoot, List content) + { + foreach (var package in content.GroupBy(c => c.Directory).OrderBy(g => g.Key, StringComparer.Ordinal)) + { + var definitions = new Dictionary(StringComparer.Ordinal); + var properties = new Dictionary<(string, string), Dictionary>(); + var jsonFiles = new List<(string Path, string Version)>(); + foreach (var entry in package.Where(c => c.ContentType.Equals("MasterData", StringComparison.OrdinalIgnoreCase))) + { + var literal = entry.Source.Replace('\\', '/').Split('/').TakeWhile(s => !s.Contains('$') && !s.Contains('*') && !s.Contains('?')); + var root = Path.Combine(entry.Directory, Path.Combine(literal.ToArray())); + if (!Directory.Exists(root)) continue; + var pattern = SourcePattern(entry.Source); + foreach (var file in Directory.EnumerateFiles(root, "*.json", SearchOption.AllDirectories)) + { + var match = pattern.Match(Path.GetRelativePath(entry.Directory, file).Replace('\\', '/')); + if (match.Success && match.Groups["version"].Success) jsonFiles.Add((file, match.Groups["version"].Value)); + } + } + foreach (var (file, version) in jsonFiles.Distinct().OrderBy(f => System.Version.Parse(f.Version)).ThenBy(f => f.Path, StringComparer.Ordinal)) + { + try + { + using var doc = JsonDocument.Parse(File.ReadAllText(file)); + if (doc.RootElement.ValueKind != JsonValueKind.Object) continue; + foreach (var section in doc.RootElement.EnumerateObject()) + { + var kind = section.Name[(section.Name.LastIndexOf('>') + 1)..]; + var tableDefinition = kind is "SmartTable" or "GenericTable"; + var tableProperty = kind is "SmartTableProperties" or "GenericTableProperties"; + if ((!tableDefinition && !tableProperty) || section.Value.ValueKind != JsonValueKind.Object) continue; + foreach (var row in section.Value.EnumerateObject().Select(p => p.Value).Where(v => v.ValueKind == JsonValueKind.Object)) + { + var table = Text(row, tableDefinition ? "Name" : kind == "SmartTableProperties" ? "SmartTable" : "GenericTable"); + if (table is null) continue; + bool changed; + if (tableDefinition) + { + changed = !definitions.ContainsKey(table); + definitions.TryAdd(table, version); + } + else + { + var name = Text(row, "Name"); + if (name is null) continue; + // Ignore description/precedence/row-data edits: they don't define table structure. + changed = !properties.TryGetValue((table, name), out var fields); + fields ??= new Dictionary(StringComparer.Ordinal); + foreach (var key in new[] { "ScalarType", "ScalarSize", "ScalarPrecision", "ReferenceType", "ReferenceName", "IsArray", "IsMandatory", "IsEnabled", "IsIndexed" }) + { + if (!row.TryGetProperty(key, out var value)) continue; + if (!fields.TryGetValue(key, out var previous) || previous != value.ToString()) changed = true; + fields[key] = value.ToString(); + } + properties[(table, name)] = fields; + } + if (changed) _schemaChanges.TryAdd((package.Key, version, table), CodeReviewPaths.GetRelativePath(repositoryRoot, file)); + } + } + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException or JsonException) + { + Console.Error.WriteLine($"warning: Process Rule schema context unavailable for '{CodeReviewPaths.GetRelativePath(repositoryRoot, file)}': {ex.Message}"); + } + } + } + } + + private static string? Text(JsonElement row, string key) => + row.TryGetProperty(key, out var value) && value.ValueKind == JsonValueKind.String ? value.GetString() : null; +} diff --git a/cmf-codereview/src/CmFrameworkScanner.cs b/cmf-codereview/src/CmFrameworkScanner.cs index 7129f85..0953fe9 100644 --- a/cmf-codereview/src/CmFrameworkScanner.cs +++ b/cmf-codereview/src/CmFrameworkScanner.cs @@ -201,7 +201,8 @@ private static IReadOnlyList BuildDefaultRules() => new ConfigGetConfigRule(), new GetConfigInsteadOfTryGetConfigRule(), new ConfigWriteInHotPathRule(), - new ApplicationContextInExtensionRule() + new ApplicationContextInExtensionRule(), + new ProcessRuleValidationRule() ]; /// Rule names for which severity can be escalated when the match sits in a nested loop or a caller-induced loop. @@ -278,6 +279,9 @@ void Mark(string phase) _ = source.MethodRanges; }); Mark("warm cache"); + var processRuleRepository = ProcessRuleRepositoryContext.Build(repositoryRoot, + CodeReviewPaths.GetFiles(repositoryRoot, CodeReviewPaths.ResolveRoots(repositoryRoot, ReviewScope.Both)).Concat(files).Distinct(StringComparer.OrdinalIgnoreCase), fileCache); + Mark("Process Rule deployment context"); var serviceEndpointMap = CmServiceEndpointResolver.GetServiceEndpointMap(repositoryRoot); Mark("service endpoints"); @@ -320,7 +324,7 @@ void Mark(string phase) var findings = filesToReport .AsParallel().AsOrdered() .SelectMany(file => FindFileFindings(file, fileCache, callGraph, loopExposureCache, settings, baseline, - serviceEndpointMap, deeClassNameByFile, deeInvocationMap, orchestrationCallSitesInDees, activeRules, repositoryRoot, entityCatalog)) + serviceEndpointMap, deeClassNameByFile, deeInvocationMap, orchestrationCallSitesInDees, activeRules, repositoryRoot, entityCatalog, processRuleRepository)) .ToList(); Mark("rule loop"); @@ -328,6 +332,7 @@ void Mark(string phase) .OrderBy(f => f.Severity switch { Severity.Critical => 0, Severity.High => 1, _ => 2 }) .ThenBy(f => f.RelativePath, StringComparer.Ordinal) .ThenBy(f => f.LineNumber) + .ThenBy(f => f.Rule, StringComparer.Ordinal) .ToList(); } @@ -335,13 +340,13 @@ private static List FindFileFindings(string file, CodeReviewFileCache f ConcurrentDictionary loopExposureCache, CodeReviewSettings? settings, CodeReviewBaseline? baseline, Dictionary serviceEndpointMap, Dictionary deeClassNameByFile, Dictionary> deeInvocationMap, Dictionary orchestrationCallSitesInDees, - IReadOnlyList activeRules, string repositoryRoot, EntityCatalog entityCatalog) + IReadOnlyList activeRules, string repositoryRoot, EntityCatalog entityCatalog, ProcessRuleRepositoryContext processRuleRepository) { var findings = new List(); { var source = fileCache.Get(file); var lines = source.Lines; - var context = RuleFileContext.Build(source, callGraph, loopExposureCache, entityCatalog); + var context = RuleFileContext.Build(source, callGraph, loopExposureCache, entityCatalog, processRuleRepository); CrossReference? deeCrossReference = null; if (context.IsDeeFile) diff --git a/cmf-codereview/src/CodeReviewPaths.cs b/cmf-codereview/src/CodeReviewPaths.cs index 3ad61f2..a3a7a05 100644 --- a/cmf-codereview/src/CodeReviewPaths.cs +++ b/cmf-codereview/src/CodeReviewPaths.cs @@ -7,6 +7,8 @@ namespace Cmf.Cli.Plugin.CodeReview; +internal record DataPackageContent(string Directory, string ContentType, string Source, int Order); + /// Resolves default folder roots and scope labels, and enumerates .cs files to scan under the repository. public static class CodeReviewPaths { @@ -58,6 +60,17 @@ public static string[] ResolveRoots(string repositoryRoot, ReviewScope scope) }; } + internal static IEnumerable DiscoverDataPackageContent(string repositoryRoot) => + FindAndParseManifests(repositoryRoot) + .Where(m => string.Equals(m.PackageType, "Data", StringComparison.OrdinalIgnoreCase)) + .OrderBy(m => m.Directory, StringComparer.Ordinal) + .SelectMany(m => m.ContentToPack.Select((c, order) => new DataPackageContent(m.Directory, c.ContentType, c.Source, order))); + + internal static bool IsProcessRuleContent(string contentType) => + contentType.Equals("EntityTypes", StringComparison.OrdinalIgnoreCase) || + contentType.Equals("ProcessRulesPre", StringComparison.OrdinalIgnoreCase) || + contentType.Equals("ProcessRulesPost", StringComparison.OrdinalIgnoreCase); + private static ResolvedRoots Discover(string repositoryRoot) { var fullRoot = Path.GetFullPath(repositoryRoot); @@ -100,13 +113,18 @@ private static ResolvedRoots DiscoverUncached(string fullRepositoryRoot) var deeRoots = manifests .Where(m => string.Equals(m.PackageType, "Data", StringComparison.OrdinalIgnoreCase)) .SelectMany(m => m.ContentToPack - .Where(c => string.Equals(c.ContentType, "DEE", StringComparison.OrdinalIgnoreCase)) + .Where(c => string.Equals(c.ContentType, "DEE", StringComparison.OrdinalIgnoreCase) || IsProcessRuleContent(c.ContentType)) .Select(c => Path.Combine(m.Directory, ExtractLiteralDirectory(c.Source))) .Where(Directory.Exists)) .Select(dir => GetRelativePath(fullRepositoryRoot, dir)) .Distinct(StringComparer.OrdinalIgnoreCase) .ToArray(); + // A DEE root often already contains its Process Rule roots. Do not scan those files twice. + deeRoots = deeRoots.Where(root => !deeRoots.Any(other => + !other.Equals(root, StringComparison.OrdinalIgnoreCase) && + root.StartsWith(other + Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase))).ToArray(); + if (orchestrationRoots.Length == 0 && deeRoots.Length == 0) { throw new CliException( @@ -146,7 +164,7 @@ private static string ExtractLiteralDirectory(string source) foreach (var segment in normalized.Split('/', StringSplitOptions.RemoveEmptyEntries)) { - if (segment.Contains('*') || segment.Contains('$')) + if (segment.Contains('*') || segment.Contains('?') || segment.Contains('$')) { break; } @@ -285,7 +303,7 @@ public static List GetFiles(string repositoryRoot, IEnumerable? files.AddRange(Directory.EnumerateFiles(fullRoot, "*.cs", SearchOption.AllDirectories)); } - return files; + return files.Distinct(StringComparer.OrdinalIgnoreCase).OrderBy(f => f, StringComparer.Ordinal).ToList(); } /// True if the file sits under a Business/<Project>.Orchestration folder, for any Cmf.<Project> naming. diff --git a/cmf-codereview/src/Models/ProcessRuleInfo.cs b/cmf-codereview/src/Models/ProcessRuleInfo.cs new file mode 100644 index 0000000..ba13b82 --- /dev/null +++ b/cmf-codereview/src/Models/ProcessRuleInfo.cs @@ -0,0 +1,5 @@ +namespace Cmf.Cli.Plugin.CodeReview.Models; + +/// Deployment classification; Services rules run on every installation. +public record ProcessRuleInfo(string? Version, string Phase, string? PackageDirectory = null, + string? ContentType = null, int ContentOrder = -1, string? PackagingProblem = null); diff --git a/cmf-codereview/src/Rules/ProcessRuleSyntax.cs b/cmf-codereview/src/Rules/ProcessRuleSyntax.cs new file mode 100644 index 0000000..f456448 --- /dev/null +++ b/cmf-codereview/src/Rules/ProcessRuleSyntax.cs @@ -0,0 +1,321 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; + +namespace Cmf.Cli.Plugin.CodeReview.Rules; + +/// Bounded, single-class syntax analysis. Unknown helpers never count as proof of a guard. +internal sealed class ProcessRuleSyntax(ClassDeclarationSyntax @class, MethodDeclarationSyntax action) +{ + public static string Name(InvocationExpressionSyntax call) => call.Expression switch + { + SimpleNameSyntax n => n.Identifier.ValueText, + _ => CodeReviewSyntaxTree.GetMemberName(call.Expression)?.Identifier.ValueText ?? string.Empty + }; + + public static ExpressionSyntax? Receiver(InvocationExpressionSyntax call) => CodeReviewSyntaxTree.GetMemberTarget(call.Expression); + public static string Tokens(SyntaxNode? node) => node is null ? string.Empty : string.Concat(node.DescendantTokens().Select(t => t.Text)); + public static ExpressionSyntax Unwrap(ExpressionSyntax expr) => expr is ParenthesizedExpressionSyntax p ? Unwrap(p.Expression) : expr; + + // Local functions are analyzed only when reachable from DeeActionCode; dormant helper bodies and + // callbacks are not deployment operations. Recursive cycles are bounded by the visited set. + public IReadOnlyList Calls { get; } = ReachableCalls(action); + + private static IReadOnlyList ReachableCalls(MethodDeclarationSyntax method) + { + var result = DirectCalls(method).ToList(); + var visited = new HashSet(); + for (var i = 0; i < result.Count; i++) + { + var call = result[i]; + if (FindLocal(call) is { } local && visited.Add(local)) result.AddRange(DirectCalls(local)); + } + return result; + } + + public static IEnumerable DirectNodes(SyntaxNode callable) => + callable.DescendantNodes(n => n == callable || n is not (LocalFunctionStatementSyntax or AnonymousFunctionExpressionSyntax)); + + private static IEnumerable DirectCalls(SyntaxNode callable) => + DirectNodes(callable).OfType(); + + private static LocalFunctionStatementSyntax? FindLocal(InvocationExpressionSyntax call) + { + if (call.Expression is not SimpleNameSyntax simple) return null; + foreach (var block in call.Ancestors().OfType()) + { + var candidates = block.Statements.OfType() + .Where(l => l.Identifier.ValueText == simple.Identifier.ValueText).ToList(); + if (candidates.Count != 0) return candidates.Count == 1 ? candidates[0] : null; + } + return null; + } + + public string Key(ExpressionSyntax? expression) + { + if (expression is null) return string.Empty; + return StringValue(expression) is { } value ? "\"" + value + "\"" : Tokens(expression); + } + + public string? StringValue(ExpressionSyntax expression, int depth = 0) + { + if (depth > 6) return null; + expression = Unwrap(expression); + if (expression is LiteralExpressionSyntax literal && literal.IsKind(SyntaxKind.StringLiteralExpression)) return literal.Token.ValueText; + if (expression is not IdentifierNameSyntax identifier) return null; + // Respect lexical scopes; an unrelated method's same-named constant is not evidence. + foreach (var scope in expression.Ancestors().Where(n => n is BlockSyntax or ClassDeclarationSyntax)) + { + var candidates = scope is BlockSyntax block + ? block.Statements.OfType().SelectMany(d => d.Declaration.Variables) + : ((ClassDeclarationSyntax)scope).Members.OfType().Where(f => f.Modifiers.Any(SyntaxKind.ConstKeyword)).SelectMany(f => f.Declaration.Variables); + var declarations = candidates.Where(d => d.Identifier.ValueText == identifier.Identifier.ValueText && + (scope is ClassDeclarationSyntax || d.SpanStart < expression.SpanStart)).ToList(); + if (declarations.Count == 0) continue; + if (declarations.Count != 1 || declarations[0].Initializer is null) return null; + var declaration = declarations[0]; + if (scope.DescendantNodes().OfType().Any(a => a.SpanStart > declaration.SpanStart && + a.SpanStart < expression.SpanStart && Tokens(a.Left) == identifier.Identifier.ValueText)) return null; + return StringValue(declaration.Initializer!.Value, depth + 1); + } + return null; + } + + public string? TypeOf(ExpressionSyntax? expression) + { + if (expression is null) return null; + expression = Unwrap(expression); + if (expression is not IdentifierNameSyntax identifier) return null; + foreach (var scope in expression.Ancestors()) + { + if (scope is BlockSyntax block) + { + var declaration = block.Statements.OfType().SelectMany(d => d.Declaration.Variables) + .FirstOrDefault(d => d.Identifier.ValueText == identifier.Identifier.ValueText); + if (declaration is not null) return DeclarationType(declaration); + } + var parameters = scope switch + { + MethodDeclarationSyntax m => m.ParameterList.Parameters, + LocalFunctionStatementSyntax l => l.ParameterList.Parameters, + _ => default + }; + var parameter = parameters.FirstOrDefault(p => p.Identifier.ValueText == identifier.Identifier.ValueText); + if (parameter?.Type is { } type) return CodeReviewSyntaxTree.LastIdentifierSegment(type.ToString()); + } + return null; + } + + private static string? DeclarationType(VariableDeclaratorSyntax variable) + { + var declared = ((VariableDeclarationSyntax)variable.Parent!).Type; + if (!declared.IsVar) return CodeReviewSyntaxTree.LastIdentifierSegment(declared.ToString()); + var initial = variable.Initializer?.Value; + if (initial is InvocationExpressionSyntax invocation) + { + var generic = invocation.Expression as GenericNameSyntax ?? CodeReviewSyntaxTree.GetMemberName(invocation.Expression) as GenericNameSyntax; + if (generic?.TypeArgumentList.Arguments.Count == 1 && Name(invocation) is "GetService" or "GetRequiredService" or "Create") + return CodeReviewSyntaxTree.LastIdentifierSegment(generic.TypeArgumentList.Arguments[0].ToString()); + } + return initial is ObjectCreationExpressionSyntax creation ? CodeReviewSyntaxTree.LastIdentifierSegment(creation.Type.ToString()) : null; + } + + public ExpressionSyntax? AssignedValue(string target, string member, SyntaxNode before) + { + var matches = @class.DescendantNodes().OfType() + .Where(a => a.SpanStart < before.SpanStart && Tokens(a.Left) == target + "." + member && + a.Ancestors().FirstOrDefault(n => n is MethodDeclarationSyntax or LocalFunctionStatementSyntax) == + before.Ancestors().FirstOrDefault(n => n is MethodDeclarationSyntax or LocalFunctionStatementSyntax)).ToList(); + return matches.Count == 1 ? matches[0].Right : null; + } + + public bool Guarded(SyntaxNode operation, string target, string? property, bool exists, int depth = 0) + { + if (depth > 4) return false; + bool Predicate(ExpressionSyntax expression, bool truth) => IsGuardPredicate(expression, truth, target, property, exists) && + !Calls.Any(c => c.SpanStart > expression.Span.End && c.SpanStart < operation.SpanStart && + Tokens(Receiver(c)) == target && Name(c) is "Create" or "Delete" or "Remove" or "Terminate") && + !@class.DescendantNodes().OfType().Any(a => a.SpanStart > expression.Span.End && + a.SpanStart < operation.SpanStart && (Tokens(a.Left) == target || Tokens(a.Left) == target + ".Name")); + if (ControlGuard(operation, Predicate)) return true; + if (operation.Ancestors().OfType().FirstOrDefault() is not { } local) return false; + var callers = Calls.Where(c => FindLocal(c) == local).ToList(); + return callers.Count > 0 && callers.All(c => Guarded(c, Substitute(target, local, c), + property is null ? null : Substitute(property, local, c), exists, depth + 1)); + } + + private string Substitute(string key, LocalFunctionStatementSyntax local, InvocationExpressionSyntax call) + { + for (var i = 0; i < local.ParameterList.Parameters.Count && i < call.ArgumentList.Arguments.Count; i++) + { + if (key == local.ParameterList.Parameters[i].Identifier.ValueText) return Key(call.ArgumentList.Arguments[i].Expression); + } + return key; + } + + public string? ParameterAlias(LocalFunctionStatementSyntax local, string target) + { + var callers = Calls.Where(c => FindLocal(c) == local).ToList(); + if (callers.Count == 0) return null; + foreach (var parameter in local.ParameterList.Parameters.Where(p => p.Type?.ToString() == "IEntityType")) + if (callers.All(c => Substitute(parameter.Identifier.ValueText, local, c) == target)) return parameter.Identifier.ValueText; + return null; + } + + private bool IsGuardPredicate(ExpressionSyntax expression, bool truth, string target, string? property, bool exists) + { + if (expression is InvocationExpressionSyntax call) + { + if (property is not null && Name(call) is "HasProperty" or "TryGetEntityTypeProperty") + return Tokens(Receiver(call)) == target && call.ArgumentList.Arguments.Count > 0 && + Key(call.ArgumentList.Arguments[0].Expression) == property && truth == exists; + if (property is null && Name(call) is "ObjectExists" or "TryLoad") return Tokens(Receiver(call)) == target && truth == exists; + } + return false; + } + + public bool TerminationGuarded(SyntaxNode operation, string target) => ControlGuard(operation, (expression, truth) => + { + if (expression is BinaryExpressionSyntax binary) + { + var member = binary.Left as MemberAccessExpressionSyntax ?? binary.Right as MemberAccessExpressionSyntax; + var other = member == binary.Left ? binary.Right : binary.Left; + if (member is not null && Tokens(member.Expression) == target && member.Name.Identifier.ValueText == "UniversalState" && + Tokens(other).EndsWith(".Terminated", StringComparison.Ordinal)) + return binary.IsKind(SyntaxKind.NotEqualsExpression) && truth || binary.IsKind(SyntaxKind.EqualsExpression) && !truth; + } + return false; + }); + + public bool HasSkippedGuard(string target, string? property, bool exists) => @class.Members.OfType() + .Where(m => m.Identifier.ValueText == "DeeTestCondition") + .SelectMany(m => DirectNodes(m).OfType()) + .Any(e => IsGuardPredicate(e, exists, target, property, exists)); + + /// A repair must remove the old property whenever it exists, not only when an unrelated flag is true. + public bool RepairConditions(SyntaxNode operation, string target, string property, string? collection = null) + { + bool Known(ExpressionSyntax expression, bool truth) + { + expression = Unwrap(expression); + if (expression is PrefixUnaryExpressionSyntax negation && negation.IsKind(SyntaxKind.LogicalNotExpression)) return Known(negation.Operand, !truth); + if (expression is BinaryExpressionSyntax binary) + { + if (binary.IsKind(SyntaxKind.LogicalAndExpression) && truth) return Known(binary.Left, true) && Known(binary.Right, true); + if (collection is not null && Tokens(binary.Left) is { } left && (left == collection + ".Count" || left == collection + "?.Count") && + binary.Right is LiteralExpressionSyntax { Token.ValueText: "0" } && truth && + (binary.IsKind(SyntaxKind.GreaterThanExpression) || binary.IsKind(SyntaxKind.NotEqualsExpression))) return true; + } + return IsGuardPredicate(expression, truth, target, property, true) || IsGuardPredicate(expression, truth, target, null, true); + } + foreach (var ancestor in operation.Ancestors()) + { + if (ancestor is MethodDeclarationSyntax or LocalFunctionStatementSyntax) break; + if (ancestor is IfStatementSyntax condition && !Known(condition.Condition, condition.Statement.Span.Contains(operation.Span))) return false; + } + return true; + } + + public static bool ControlGuard(SyntaxNode operation, Func predicate) + { + for (SyntaxNode? node = operation; node?.Parent is { } parent; node = parent) + { + if (parent is LocalFunctionStatementSyntax or MethodDeclarationSyntax or AnonymousFunctionExpressionSyntax) break; + if (parent is IfStatementSyntax condition && condition.Statement.Span.Contains(node.Span) && Guarantees(condition.Condition, true, predicate)) return true; + if (parent is ElseClauseSyntax alternative && alternative.Parent is IfStatementSyntax other && Guarantees(other.Condition, false, predicate)) return true; + if (parent is BlockSyntax block) + { + var containing = block.Statements.FirstOrDefault(s => s.Span.Contains(node.Span)); + foreach (var previous in block.Statements.TakeWhile(s => s != containing)) + { + if (previous is IfStatementSyntax early && + (Exits(early.Statement) && Guarantees(early.Condition, false, predicate) || + early.Else is { } @else && Exits(@else.Statement) && Guarantees(early.Condition, true, predicate))) return true; + } + } + } + return false; + } + + private static bool Guarantees(ExpressionSyntax expression, bool truth, Func predicate) + { + expression = Unwrap(expression); + if (expression is PrefixUnaryExpressionSyntax negation && negation.IsKind(SyntaxKind.LogicalNotExpression)) return Guarantees(negation.Operand, !truth, predicate); + if (expression is BinaryExpressionSyntax binary) + { + if (binary.IsKind(SyntaxKind.LogicalAndExpression)) return truth + ? Guarantees(binary.Left, true, predicate) || Guarantees(binary.Right, true, predicate) + : Guarantees(binary.Left, false, predicate) && Guarantees(binary.Right, false, predicate); + if (binary.IsKind(SyntaxKind.LogicalOrExpression)) return truth + ? Guarantees(binary.Left, true, predicate) && Guarantees(binary.Right, true, predicate) + : Guarantees(binary.Left, false, predicate) || Guarantees(binary.Right, false, predicate); + if (binary.IsKind(SyntaxKind.EqualsExpression) || binary.IsKind(SyntaxKind.NotEqualsExpression)) + { + var literal = binary.Right as LiteralExpressionSyntax ?? binary.Left as LiteralExpressionSyntax; + if (literal is not null && (literal.IsKind(SyntaxKind.TrueLiteralExpression) || literal.IsKind(SyntaxKind.FalseLiteralExpression))) + { + var operand = literal == binary.Right ? binary.Left : binary.Right; + var desired = truth == binary.IsKind(SyntaxKind.EqualsExpression) == literal.IsKind(SyntaxKind.TrueLiteralExpression); + return Guarantees(operand, desired, predicate); + } + } + } + return predicate(expression, truth); + } + + private static bool Exits(StatementSyntax statement) => statement switch + { + ReturnStatementSyntax or ThrowStatementSyntax => true, + BlockSyntax block => block.Statements.Any(Exits), + IfStatementSyntax conditional => Exits(conditional.Statement) && conditional.Else is { } other && Exits(other.Statement), + _ => false + }; + + /// True for recognized calls that must occur after this operation in the same execution path. + public bool HasFollowingCall(SyntaxNode operation, Func predicate) + { + foreach (var block in operation.Ancestors().OfType()) + { + if (block.Ancestors().OfType().FirstOrDefault() != operation.Ancestors().OfType().FirstOrDefault()) break; + var statement = block.Statements.FirstOrDefault(s => s.Span.Contains(operation.Span)); + if (statement is null) continue; + foreach (var next in block.Statements.SkipWhile(s => s != statement).Skip(1)) + { + if (next is ReturnStatementSyntax or ThrowStatementSyntax) return false; + if (DirectNodes(next).Any(n => n is ReturnStatementSyntax or ThrowStatementSyntax)) return false; + if (next is ExpressionStatementSyntax or LocalDeclarationStatementSyntax) + if (DirectCalls(next).Any(predicate)) return true; + } + } + return false; + } + + public bool CleanupMatches(InvocationExpressionSyntax call, string property) => Name(call) == "RemoveAttributes" && + call.ArgumentList.Arguments.Any(a => ContainsProperty(a.Expression, property)); + + private bool ContainsProperty(ExpressionSyntax expression, string property) + { + if (expression.DescendantNodesAndSelf().OfType().Any(e => Key(e) == property)) return true; + if (expression is not IdentifierNameSyntax identifier) return false; + var variables = expression.Ancestors().OfType().SelectMany(b => b.Statements.OfType()) + .SelectMany(d => d.Declaration.Variables).Where(v => v.Identifier.ValueText == identifier.Identifier.ValueText).ToList(); + return variables.Count == 1 && variables[0].Initializer?.Value is BaseObjectCreationExpressionSyntax creation && + creation.Initializer?.Expressions.Any(e => Key(e) == property) == true; + } + + public bool HasPriorCleanup(SyntaxNode operation, string property) => Calls.Any(c => CleanupMatches(c, property) && + c.SpanStart < operation.SpanStart && + SameExecutionScope(c, operation)); + + private static bool SameExecutionScope(SyntaxNode earlier, SyntaxNode later) + { + var earlierMethod = earlier.Ancestors().FirstOrDefault(n => n is LocalFunctionStatementSyntax or MethodDeclarationSyntax); + if (earlierMethod != later.Ancestors().FirstOrDefault(n => n is LocalFunctionStatementSyntax or MethodDeclarationSyntax)) return false; + if (earlier.Ancestors().OfType().Any(t => t.Catches.Count > 0 && + t.Block.Span.Contains(earlier.Span) && !t.Block.Span.Contains(later.Span))) return false; + // Conditional cleanup counts only when the removal is in that same branch. A loop over holders + // may be empty: in that case there was no value to clear, so it is a valid cleanup shape. + return earlier.Ancestors().OfType().All(i => + i.Statement.Span.Contains(earlier.Span) ? i.Statement.Span.Contains(later.Span) : i.Else?.Span.Contains(later.Span) == true); + } +} diff --git a/cmf-codereview/src/Rules/ProcessRuleValidationRule.cs b/cmf-codereview/src/Rules/ProcessRuleValidationRule.cs new file mode 100644 index 0000000..25db153 --- /dev/null +++ b/cmf-codereview/src/Rules/ProcessRuleValidationRule.cs @@ -0,0 +1,268 @@ +using Cmf.Cli.Plugin.CodeReview.Enums; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using static Cmf.Cli.Plugin.CodeReview.Rules.ProcessRuleSyntax; + +namespace Cmf.Cli.Plugin.CodeReview.Rules; + +/// Deterministic deployment checks derived from Process Rule authoring conventions. +public sealed class ProcessRuleValidationRule : IReviewRule +{ + public IEnumerable EscalatableRuleNames => Array.Empty(); + + public IEnumerable FindMatches(RuleFileContext context) + { + if (context.ProcessRule is not { } deployment) yield break; + foreach (var @class in context.Root.DescendantNodes().OfType() + .Where(c => c.BaseList?.Types.Any(t => CodeReviewSyntaxTree.LastIdentifierSegment(t.Type.ToString()) == "DeeDevBase") == true)) + { + var action = @class.Members.OfType().FirstOrDefault(m => m.Identifier.ValueText == "DeeActionCode" && m.Modifiers.Any(SyntaxKind.OverrideKeyword)); + if (action is null) continue; + var condition = @class.Members.OfType().FirstOrDefault(m => m.Identifier.ValueText == "DeeTestCondition" && m.Modifiers.Any(SyntaxKind.OverrideKeyword)); + var conditionHasLogic = condition is not null && !ReturnsTrue(condition); + if (deployment.PackagingProblem is { } packagingProblem) + yield return Match("ProcessRulePlacement", @class, Severity.High, packagingProblem, + "Align the Process Rule content type and position in cmfpackage.json with its deployment phase."); + foreach (var problem in context.ProcessRuleOrderProblems) + yield return Match("ProcessRuleDependencyOrder", @class, Severity.High, problem, + "Run the entity-type creator before consumers; use the proper content phase or deterministic alphabetical prefixes for sibling rules."); + if (conditionHasLogic) + yield return Match("ProcessRuleCondition", condition!, Severity.High, + "Deployment calls DeeActionCode directly and does not evaluate DeeTestCondition. This condition contains logic that cannot protect installation.", + "Make DeeTestCondition simply return true; put each existence/state guard around its operation in DeeActionCode."); + + var syntax = new ProcessRuleSyntax(@class, action); + var calls = syntax.Calls; + foreach (var call in calls) + { + var name = Name(call); + if (name == "CreateAttribute" && call.ArgumentList.Arguments.Count > 0) + { + var entity = AttributeEntity(syntax, call); + var property = syntax.Key(call.ArgumentList.Arguments[0].Expression); + if (entity is not null && !syntax.Guarded(call, entity, property, false) && !RemovedFirst(syntax, call, entity, property)) + yield return MutationMatch(syntax, call, entity, property, false, conditionHasLogic); + else if (entity is null) + yield return Match("ProcessRuleUnresolvedGuard", call, Severity.Medium, + "CreateAttribute target entity type cannot be resolved by the supported single-file patterns.", + "Verify a matching attribute existence guard in DeeActionCode and trace which entity type receives this property."); + } + + if (name == "FullUpdateEntityType") + { + var entity = InputMember(call, "EntityType"); + if (entity is null) continue; + var target = Tokens(entity); + var adds = InputMember(call, "EntityTypePropertiesToAddOrUpdate"); + var removes = InputMember(call, "EntityTypePropertiesToRemoveOrTerminate"); + if ((adds is not null || removes is not null) && !syntax.HasFollowingCall(call, c => IsSchemaCall(c, target))) + yield return SchemaMatch(call, target); + if (adds is not null) + foreach (var match in CheckManualProperties(syntax, call, adds, target, conditionHasLogic)) yield return match; + if (removes is not null) + { + var removedProperties = CollectionProperties(syntax, removes, call).ToList(); + if (removedProperties.Count == 0) + yield return Match("ProcessRuleAttributeRemovalReview", call, Severity.Medium, + "Removed entity-type properties cannot be resolved from local TryGetEntityTypeProperty/collection-add patterns.", + "Verify existence guards, value preservation/cleanup before removal, and schema regeneration for every property supplied by the helper/collection."); + foreach (var item in removedProperties) + { + if (!syntax.Guarded(item.Source, item.Entity, item.Property, true)) + yield return MutationMatch(syntax, item.Source, item.Entity, item.Property, true, conditionHasLogic); + if (!syntax.HasPriorCleanup(call, item.Property)) + yield return Match("ProcessRuleAttributeRemoval", call, + calls.Any(c => c.SpanStart > call.SpanStart && syntax.CleanupMatches(c, item.Property)) ? Severity.Critical : Severity.High, + $"No matching RemoveAttributes cleanup is recognized before removing {item.Entity}.{item.Property}. Populated attributes can prevent deployment or lose data during a repair.", + "Query all holders of the attribute, preserve values if required, clear them before removing the property, and regenerate schema. Review external cleanup helpers and suppress only after verification."); + } + } + } + + if (name == "Create" && syntax.TypeOf(Receiver(call)) is "IEntityTypePropertyCollection" or "EntityTypePropertyCollection") + { + var collection = Receiver(call)!; + var properties = ManualProperties(syntax, collection, call).ToList(); + foreach (var item in properties) + if (!syntax.Guarded(item.Source, item.Entity, item.Property, false) && !RemovedFirst(syntax, call, item.Entity, item.Property)) + yield return MutationMatch(syntax, item.Source, item.Entity, item.Property, false, conditionHasLogic); + var targets = properties.Select(p => p.Entity).Distinct().ToList(); + if (targets.Count == 1 && !syntax.HasFollowingCall(call, c => IsSchemaCall(c, targets[0]))) yield return SchemaMatch(call, targets[0]); + } + + var receiver = Receiver(call); + var type = syntax.TypeOf(receiver); + if (type is "ISmartTable" or "SmartTable" or "IGenericTable" or "GenericTable" && + name is "LoadData" or "InsertOrUpdateRows" or "InsertRows" or "UpdateRows" or "DeleteRows") + { + var tableName = TableName(syntax, receiver!, call); + var schemaSource = tableName is null ? null : context.ProcessRuleRepository?.GetSchemaChange(deployment, tableName); + if (schemaSource is not null) + yield return Match("ProcessRuleTableMigration", call, Severity.Critical, + $"Table '{tableName}' is created or structurally changed in package version {deployment.Version} ({schemaSource}); '{name}' uses it within the deployment transaction.", + "Move data access/migration to a documented post-install step after the transaction commits, or to a later independent package. After-phase placement alone does not make it safe."); + else if (name is "InsertOrUpdateRows" or "InsertRows" or "UpdateRows" or "DeleteRows" && + (tableName is null || deployment.Version is null || deployment.PackageDirectory is null)) + yield return Match("ProcessRuleTableMigrationReview", call, Severity.Medium, + "Table mutation has an unresolved table name or package version. Syntax analysis cannot establish whether it depends on schema changed in this deployment.", + "Verify the table structure is unchanged in this version and the action is safe on reruns; move same-version schema/data migrations to a post-install step."); + } + + if (receiver is not null && type is "IAction" or "Action" or "IRule" or "Rule" or "IConfig" or "Config" or "IEntityType" or "EntityType" && + name is "Create" or "Delete" or "Remove" or "Terminate" or "Save" or "RemoveActionGroups") + { + var target = Tokens(receiver); + if (!syntax.Guarded(call, target, null, name != "Create")) + yield return MutationMatch(syntax, call, target, null, name != "Create", conditionHasLogic); + if (name == "Terminate" && !syntax.TerminationGuarded(call, target)) + yield return Match("ProcessRuleTerminationState", call, Severity.High, + $"'{target}' is terminated without a recognized already-terminated state guard. Existence alone does not make termination safe on retries.", + "Check existence and UniversalState != Terminated before terminating the same rule/entity."); + } + if (name == "RemoveAction" && InputMember(call, "Action") is { } removedAction && + !syntax.Guarded(call, Tokens(removedAction), null, true)) + yield return MutationMatch(syntax, call, Tokens(removedAction), null, true, conditionHasLogic); + } + + var propertyChange = calls.FirstOrDefault(c => Name(c) is "CreateAttribute" or "FullUpdateEntityType" || + Name(c) == "Create" && syntax.TypeOf(Receiver(c)) is "IEntityTypePropertyCollection" or "EntityTypePropertyCollection"); + if (deployment.Phase is "Before" or "After" && propertyChange is not null) + yield return Match("ProcessRulePlacement", propertyChange, Severity.High, + "Entity-type property changes are in a general Before/After folder rather than the dedicated EntityTypesAttributes deployment content.", + "Use EntityTypesAttributes/ for attribute-only changes, or EntityTypes/ for creation of a complete entity type. Verify the package content ordering."); + if (deployment.Phase == "Unknown") + yield return Match("ProcessRulePlacement", @class, Severity.High, + "Process Rule path does not match a versioned Before/After, EntityTypes, EntityTypesAttributes, or recurring Services location.", + "Place the file in an appropriate deployment folder and include it in the package manifest."); + if (deployment.Version is { } version) + { + var namespaceName = string.Join(".", @class.Ancestors().OfType().Reverse().Select(n => n.Name.ToString())); + var segments = namespaceName.Split('.'); + if (!segments.Contains("_" + version.Replace('.', '_'))) + yield return Match("ProcessRuleVersionNamespace", @class, Severity.Medium, + $"Namespace does not contain the version segment _{version.Replace('.', '_')} matching deployment folder {version}.", + "Use dotted versions in folders and underscore-separated versions in the Process Rule namespace."); + if (deployment.Phase is "Before" or "After" && segments.LastOrDefault() != deployment.Phase) + yield return Match("ProcessRulePlacement", @class, Severity.High, + $"Namespace phase does not match the {deployment.Phase} deployment folder.", + "Align the namespace phase with the folder and package content type."); + } + if (deployment.Phase == "Services" && calls.FirstOrDefault(c => Name(c) is "CreateAttribute" or "FullUpdateEntityType" or + "RemoveAction" or "InsertOrUpdateRows" or "InsertRows" or "DeleteRows" || + Name(c) is "Create" or "Delete" or "Remove" or "Terminate" && syntax.TypeOf(Receiver(c)) is "IAction" or "IRule" or "IConfig" or "IEntityType") is { } recurringMutation) + yield return Match("ProcessRuleRecurringMigration", recurringMutation, Severity.Medium, + "This migration-like mutation is under unversioned Services and will be repackaged and run on every deployment.", + "Use a versioned Process Rule for a one-time migration; keep this here only if recurring execution is intentional and idempotent."); + } + } + + private static bool ReturnsTrue(MethodDeclarationSyntax method) => + method.ExpressionBody is { } arrow && Unwrap(arrow.Expression).IsKind(SyntaxKind.TrueLiteralExpression) || + method.Body?.Statements is { Count: 1 } statements && statements[0] is ReturnStatementSyntax { Expression: { } value } && Unwrap(value).IsKind(SyntaxKind.TrueLiteralExpression); + + private static RuleMatch Match(string id, SyntaxNode node, Severity severity, string description, string fix) => + new(id, CodeReviewSyntaxTree.StartLine(node), severity, description, fix); + + private static RuleMatch MutationMatch(ProcessRuleSyntax syntax, InvocationExpressionSyntax call, string target, string? property, bool exists, bool conditionHasLogic) + { + var unknownGuard = ControlGuard(call, (e, _) => e is InvocationExpressionSyntax c && Name(c) is not ("HasProperty" or "TryGetEntityTypeProperty" or "ObjectExists" or "TryLoad")); + var skippedGuard = conditionHasLogic && syntax.HasSkippedGuard(target, property, exists); + var severity = unknownGuard ? Severity.Medium : skippedGuard ? Severity.Critical : Severity.High; + return Match(unknownGuard ? "ProcessRuleUnresolvedGuard" : "ProcessRuleUnguardedMutation", call, severity, + $"'{Name(call)}' has no recognized target-specific {(exists ? "existence/state" : "absence")} guard for {target}{(property is null ? "" : "." + property)} in DeeActionCode." + + (unknownGuard ? " The enclosing helper condition needs review." : skippedGuard ? " The matching DeeTestCondition guard is skipped during deployment." : " Re-execution can fail when the target state already exists."), + "Guard the operation in DeeActionCode using the same target/property (HasProperty, TryGetEntityTypeProperty, ObjectExists, TryLoad, or termination-state checks). Verify helper behavior and both initial/already-applied cases."); + } + + private static RuleMatch SchemaMatch(SyntaxNode node, string target) => Match("ProcessRuleSchemaRegeneration", node, Severity.High, + $"Raw entity-type property update for '{target}' has no recognized subsequent schema regeneration on the same execution path.", + "Call GenerateEntityTypeDBSchema for the same entity type or its GenerateSchema method after the update. IMicronEntityTypeService.UpdateEntityTypeProperties already handles regeneration."); + + private static bool IsSchemaCall(InvocationExpressionSyntax call, string target) => + Name(call) == "GenerateSchema" && Tokens(Receiver(call)) == target || + Name(call) == "GenerateEntityTypeDBSchema" && Tokens(InputMember(call, "EntityType")) == target; + + private static ExpressionSyntax? InputMember(InvocationExpressionSyntax call, string name) => call.ArgumentList.Arguments + .Select(a => a.Expression).OfType() + .SelectMany(c => c.Initializer?.Expressions.OfType() ?? []) + .FirstOrDefault(a => Tokens(a.Left) == name)?.Right; + + private static string? AttributeEntity(ProcessRuleSyntax syntax, InvocationExpressionSyntax creation) + { + var local = creation.Ancestors().OfType().FirstOrDefault(); + if (creation.Ancestors().OfType().FirstOrDefault(c => Name(c) == "Add") is { } add) + { + var collection = Tokens(Receiver(add)); + var entities = syntax.Calls.Where(c => Name(c) == "UpdateEntityTypeProperties" && c.ArgumentList.Arguments.Count >= 2 && + Tokens(c.ArgumentList.Arguments[1].Expression) == collection) + .Select(c => Tokens(c.ArgumentList.Arguments[0].Expression)) + .Concat(syntax.Calls.Where(c => Name(c) == "FullUpdateEntityType" && Tokens(InputMember(c, "EntityTypePropertiesToAddOrUpdate")) == collection) + .Select(c => Tokens(InputMember(c, "EntityType")))) + .Where(s => s.Length != 0).Distinct().ToList(); + if (entities.Count == 1) return local is null ? entities[0] : syntax.ParameterAlias(local, entities[0]) ?? entities[0]; + } + if (local is not null) + { + var entityParameters = local.ParameterList.Parameters.Where(p => p.Type?.ToString() == "IEntityType").ToList(); + if (entityParameters.Count == 1) return entityParameters[0].Identifier.ValueText; + } + var entityVariables = creation.Ancestors().OfType().FirstOrDefault()?.DescendantNodes().OfType() + .Where(v => v.Parent is VariableDeclarationSyntax d && d.Type.ToString() == "IEntityType") + .Select(v => v.Identifier.ValueText).Distinct().ToList(); + return entityVariables?.Count == 1 ? entityVariables[0] : null; + } + + private record AttributeProperty(string Entity, string PropertyName, InvocationExpressionSyntax Source) + { + public string Property => PropertyName; + } + + private static IEnumerable CollectionProperties(ProcessRuleSyntax syntax, ExpressionSyntax collection, InvocationExpressionSyntax operation) + { + foreach (var add in syntax.Calls.Where(c => Name(c) == "Add" && Tokens(Receiver(c)) == Tokens(collection) && c.SpanStart < operation.SpanStart && c.ArgumentList.Arguments.Count == 1)) + { + var propertyVariable = Tokens(add.ArgumentList.Arguments[0].Expression); + var checks = syntax.Calls.Where(c => Name(c) == "TryGetEntityTypeProperty" && c.ArgumentList.Arguments.Count > 1 && + (Tokens(c.ArgumentList.Arguments[1].Expression) == propertyVariable || + c.ArgumentList.Arguments[1].Expression is DeclarationExpressionSyntax declaration && Tokens(declaration.Designation) == propertyVariable)).ToList(); + if (checks.Count == 1) + yield return new(Tokens(Receiver(checks[0])), syntax.Key(checks[0].ArgumentList.Arguments[0].Expression), add); + } + } + + private static IEnumerable ManualProperties(ProcessRuleSyntax syntax, ExpressionSyntax collection, InvocationExpressionSyntax operation) + { + foreach (var add in syntax.Calls.Where(c => Name(c) == "Add" && Tokens(Receiver(c)) == Tokens(collection) && c.SpanStart < operation.SpanStart && c.ArgumentList.Arguments.Count == 1)) + { + var variable = Tokens(add.ArgumentList.Arguments[0].Expression); + if (syntax.Calls.Any(c => Name(c) == "TryGetEntityTypeProperty" && c.ArgumentList.Arguments.Count > 1 && + (Tokens(c.ArgumentList.Arguments[1].Expression) == variable || c.ArgumentList.Arguments[1].Expression is DeclarationExpressionSyntax d && Tokens(d.Designation) == variable))) continue; + var name = syntax.AssignedValue(variable, "Name", add); + var entityId = syntax.AssignedValue(variable, "EntityTypeId", add) as MemberAccessExpressionSyntax; + if (name is not null && entityId?.Name.Identifier.ValueText == "Id") + yield return new(Tokens(entityId.Expression), syntax.Key(name), add); + } + } + + private static IEnumerable CheckManualProperties(ProcessRuleSyntax syntax, InvocationExpressionSyntax operation, ExpressionSyntax collection, string target, bool conditionHasLogic) + { + foreach (var item in ManualProperties(syntax, collection, operation)) + if (!syntax.Guarded(item.Source, target, item.Property, false) && !RemovedFirst(syntax, operation, target, item.Property)) + yield return MutationMatch(syntax, item.Source, target, item.Property, false, conditionHasLogic); + } + + private static bool RemovedFirst(ProcessRuleSyntax syntax, InvocationExpressionSyntax operation, string target, string property) => + syntax.Calls.Where(c => Name(c) == "FullUpdateEntityType" && c.SpanStart < operation.SpanStart && Tokens(InputMember(c, "EntityType")) == target) + .Any(c => InputMember(c, "EntityTypePropertiesToRemoveOrTerminate") is { } removed && + syntax.RepairConditions(c, target, property, Tokens(removed)) && + CollectionProperties(syntax, removed, c).Any(p => p.Entity == target && p.Property == property && + syntax.Guarded(p.Source, target, property, true) && syntax.RepairConditions(p.Source, target, property))); + + private static string? TableName(ProcessRuleSyntax syntax, ExpressionSyntax receiver, InvocationExpressionSyntax before) + { + if (syntax.AssignedValue(Tokens(receiver), "Name", before) is { } assigned) return syntax.StringValue(assigned); + var loads = syntax.Calls.Where(c => c.SpanStart < before.SpanStart && Name(c) == "Load" && Tokens(Receiver(c)) == Tokens(receiver) && c.ArgumentList.Arguments.Count > 0).ToList(); + return loads.Count == 1 ? syntax.StringValue(loads[0].ArgumentList.Arguments[0].Expression) : null; + } +} diff --git a/cmf-codereview/src/Rules/RuleFileContext.cs b/cmf-codereview/src/Rules/RuleFileContext.cs index b200c4c..ec0f880 100644 --- a/cmf-codereview/src/Rules/RuleFileContext.cs +++ b/cmf-codereview/src/Rules/RuleFileContext.cs @@ -17,6 +17,9 @@ public class RuleFileContext public required bool IsOrchestrationFile { get; init; } public required bool IsDeeFile { get; init; } public required bool IsOneTimeMigrationFile { get; init; } + public ProcessRuleInfo? ProcessRule { get; init; } + public ProcessRuleRepositoryContext? ProcessRuleRepository { get; init; } + public IReadOnlyList ProcessRuleOrderProblems { get; init; } = Array.Empty(); public required bool ContainsExtensionMethod { get; init; } /// The repository's reflection-resolved entity catalog, shared across every file's context in a scan. @@ -58,11 +61,13 @@ public static RuleFileContext Build(IReadOnlyList lines, string file, Me } /// Builds from a scan's shared per-file cache, reusing its parse and derived analyses instead of re-doing them. - public static RuleFileContext Build(CachedSourceFile source, MethodCallGraph callGraph, ConcurrentDictionary loopExposureCache, EntityCatalog entityCatalog) => - Build(source.Root, source.Lines, source.LoopState, source.MethodRanges, source.Path, callGraph, loopExposureCache, entityCatalog); + public static RuleFileContext Build(CachedSourceFile source, MethodCallGraph callGraph, ConcurrentDictionary loopExposureCache, EntityCatalog entityCatalog, + ProcessRuleRepositoryContext? processRuleRepository = null) => + Build(source.Root, source.Lines, source.LoopState, source.MethodRanges, source.Path, callGraph, loopExposureCache, entityCatalog, processRuleRepository); private static RuleFileContext Build(CompilationUnitSyntax root, IReadOnlyList lines, LoopState loopState, List methodRanges, - string file, MethodCallGraph callGraph, ConcurrentDictionary loopExposureCache, EntityCatalog entityCatalog) + string file, MethodCallGraph callGraph, ConcurrentDictionary loopExposureCache, EntityCatalog entityCatalog, + ProcessRuleRepositoryContext? processRuleRepository = null) { var isExtensionFile = CodeReviewPaths.IsExtensionFile(file); var isOrchestrationFile = CodeReviewPaths.IsOrchestrationFile(file); @@ -87,6 +92,9 @@ private static RuleFileContext Build(CompilationUnitSyntax root, IReadOnlyList(), ContainsExtensionMethod = containsExtensionMethod, EntityCatalog = entityCatalog, ConfigVarNames = configVarNames, diff --git a/cmf-codereview/tests/ProcessRuleIntegrationTests.cs b/cmf-codereview/tests/ProcessRuleIntegrationTests.cs new file mode 100644 index 0000000..aa78aad --- /dev/null +++ b/cmf-codereview/tests/ProcessRuleIntegrationTests.cs @@ -0,0 +1,210 @@ +using Cmf.Cli.Plugin.CodeReview; +using Cmf.Cli.Plugin.CodeReview.Commands; +using Cmf.Cli.Plugin.CodeReview.Enums; +using Cmf.Cli.Plugin.CodeReview.Models; +using Xunit; + +namespace codereview.tests; + +public sealed class ProcessRuleIntegrationTests : IDisposable +{ + private readonly string _root = Path.Combine(Path.GetTempPath(), "process-rule-tests-" + Guid.NewGuid()); + private string Package => Path.Combine(_root, "Cmf.Custom.Data"); + + public ProcessRuleIntegrationTests() + { + Directory.CreateDirectory(Package); + File.WriteAllText(Path.Combine(Package, "cmfpackage.json"), """ + { "packageType": "Data", "version": "2.59.0", "contentToPack": [ + { "contentType": "EntityTypes", "source": "DEEs/ProcessRules/EntityTypes/$(version)/*" }, + { "contentType": "ProcessRulesPre", "source": "DEEs/ProcessRules/$(version)/Before/*" }, + { "contentType": "MasterData", "source": "MasterData/$(version)/*" }, + { "contentType": "ProcessRulesPost", "source": "DEEs/ProcessRules/$(version)/After/*" }, + { "contentType": "ProcessRulesPost", "source": "DEEs/ProcessRules/EntityTypesAttributes/$(version)/*" }, + { "contentType": "ProcessRulesPost", "source": "DEEs/ProcessRules/Services/*" } + ] } + """); + } + + public void Dispose() => Directory.Delete(_root, recursive: true); + + private string Write(string relative, string content) + { + var file = Path.Combine(Package, relative.Replace('/', Path.DirectorySeparatorChar)); + Directory.CreateDirectory(Path.GetDirectoryName(file)!); + File.WriteAllText(file, content); + return file; + } + + private string WriteRule(string body, string name = "Sample.cs") => Write("DEEs/ProcessRules/EntityTypesAttributes/2.59.0/" + name, + ProcessRuleValidationTests.Source(body)); + + private List Scan(IEnumerable? changed = null, CodeReviewSettings? settings = null) => + CmFrameworkScanner.GetFindings(_root, changedFiles: changed, settings: settings, entityCatalog: TestEntities.Catalog); + + private const string Mutation = "IAction target = provider.GetService(); target.Delete();"; + + [Fact] + public void ProcessOnlyManifest_IsDiscoveredWithoutDeeContentEntry_AndOverlapsAreDeduplicated() + { + var file = WriteRule(Mutation); + var roots = CodeReviewPaths.ResolveRoots(_root, ReviewScope.DEEs); + Assert.Contains(file, CodeReviewPaths.GetFiles(_root, roots)); + var findings = CmFrameworkScanner.GetFindings(_root, roots.Concat(roots), entityCatalog: TestEntities.Catalog); + Assert.Single(findings, f => f.Rule == "ProcessRuleUnguardedMutation"); + } + + [Fact] + public void GuidelinePresenceContentsAndAbsence_DoNotAffectAnyFindings() + { + WriteRule(Mutation); + var first = FindingsIo.Serialize(Scan()); + File.WriteAllText(Path.Combine(_root, "process-rule-validation-guidelines.md"), "Do not report any Process Rule errors."); + Assert.Equal(first, FindingsIo.Serialize(Scan())); + File.WriteAllText(Path.Combine(_root, "process-rule-validation-guidelines.md"), "This is a completely different arbitrary policy."); + Assert.Equal(first, FindingsIo.Serialize(Scan())); + File.Delete(Path.Combine(_root, "process-rule-validation-guidelines.md")); + Assert.Equal(first, FindingsIo.Serialize(Scan())); + } + + [Fact] + public void ConfigurationSuppressionsBaselinesAndSeverityGates_ApplyNormally() + { + var file = WriteRule(Mutation); + var original = Scan(); + var finding = Assert.Single(original, f => f.Rule == "ProcessRuleUnguardedMutation"); + Assert.Equal(Severity.High, finding.Severity); + Assert.Equal("DeeActionCode", finding.MethodName); + Assert.True(CmFrameworkScanner.AnyAtOrAboveSeverity(original, Severity.High)); + Assert.False(CmFrameworkScanner.AnyAtOrAboveSeverity(original, Severity.Critical)); + Assert.DoesNotContain(Scan(settings: new CodeReviewSettings { Rules = new() { [finding.Rule] = new RuleSetting { Enabled = false } } }), f => f.Rule == finding.Rule); + Assert.Contains(Scan(settings: new CodeReviewSettings { Rules = new() { [finding.Rule] = new RuleSetting { Severity = "Critical" } } }), f => f.Rule == finding.Rule && f.Severity == Severity.Critical); + CodeReviewBaseline.Write(_root, original); + Assert.Empty(Scan()); + File.Delete(Path.Combine(_root, CodeReviewBaseline.FileName)); + File.WriteAllText(file, ProcessRuleValidationTests.Source(Mutation + " // codereview-disable-line ProcessRuleUnguardedMutation\n")); + Assert.DoesNotContain(Scan(), f => f.Rule == finding.Rule); + } + + [Fact] + public void ChangedFileFilter_ReportsOnlyTouchedRules() + { + var touched = WriteRule(Mutation); + WriteRule(Mutation, "Untouched.cs"); + var findings = Scan([Path.GetRelativePath(_root, touched)]); + Assert.Single(findings, f => f.Rule == "ProcessRuleUnguardedMutation"); + Assert.All(findings, f => Assert.Equal(touched, f.FilePath)); + } + + [Fact] + public void FindingsSerializeAndRender_ThroughExistingReports() + { + WriteRule(Mutation); + var findings = Scan(); + var json = FindingsIo.Serialize(findings); + Assert.Equal(findings.Count, System.Text.Json.JsonSerializer.Deserialize>(json)!.Count); + var path = Path.Combine(_root, "findings.json"); + File.WriteAllText(path, json); + Assert.Contains("ProcessRuleUnguardedMutation", ReportCommand.BuildContent(path, ReportMode.Detailed, ReportFormat.Markdown, "Review", "Summary")); + Assert.Contains("ProcessRuleUnguardedMutation", ReportCommand.BuildContent(path, ReportMode.Detailed, ReportFormat.Html, "Review", "Summary")); + } + + private const string TableWrite = """ + ISmartTable table = provider.GetService(); + const string TableName = "CustomTarget"; + table.Name = TableName; + table.Load(); + table.InsertOrUpdateRows(data); + """; + + [Fact] + public void SameVersionTableDefinition_ProducesCriticalFinding_WithMasterDataEvidence() + { + var rule = WriteRule(TableWrite); + var masterData = Write("MasterData/2.59.0/Schema.json", """{ "SmartTable": { "1": { "Name": "CustomTarget" } } }"""); + var finding = Assert.Single(Scan([rule]), f => f.Rule == "ProcessRuleTableMigration"); + Assert.Equal(Severity.Critical, finding.Severity); + Assert.Contains(Path.GetRelativePath(_root, masterData), finding.Description); + Assert.Contains("2.59.0", finding.Description); + } + + [Fact] + public void ExistingUnchangedTableAndDataRows_DoNotBecomeSchemaHazards() + { + WriteRule(TableWrite); + Write("MasterData/2.58.0/Schema.json", """{ "SmartTable": { "1": { "Name": "CustomTarget" } }, "SmartTableProperties": { "1": { "SmartTable": "CustomTarget", "Name": "Value", "ScalarType": "String" } } }"""); + Write("MasterData/2.59.0/Rows.json", """{ "SmartTable": { "1": { "Name": "CustomTarget", "Description": "new description" } }, "SmartTableProperties": { "1": { "SmartTable": "CustomTarget", "Name": "Value", "ScalarType": "String", "Description": "updated description" } }, "CustomTarget": { "1": { "Value": "static data" } } }"""); + Assert.DoesNotContain(Scan(), f => f.Rule == "ProcessRuleTableMigration"); + } + + [Fact] + public void PartialPropertyDescriptionUpdate_DoesNotBecomeSchemaHazard() + { + WriteRule(TableWrite); + Write("MasterData/2.58.0/Schema.json", """{ "SmartTableProperties": { "1": { "SmartTable": "CustomTarget", "Name": "Value", "ScalarType": "String" } } }"""); + Write("MasterData/2.59.0/Description.json", """{ "SmartTableProperties": { "1": { "SmartTable": "CustomTarget", "Name": "Value", "Description": "new description" } } }"""); + Assert.DoesNotContain(Scan(), f => f.Rule == "ProcessRuleTableMigration"); + } + + [Fact] + public void IncorrectManifestPhaseAndMasterDataOrder_AreFlagged() + { + File.WriteAllText(Path.Combine(Package, "cmfpackage.json"), """ + { "packageType": "Data", "contentToPack": [ + { "contentType": "ProcessRulesPre", "source": "DEEs/ProcessRules/$(version)/After/*" }, + { "contentType": "ProcessRulesPost", "source": "DEEs/ProcessRules/$(version)/Before/*" }, + { "contentType": "MasterData", "source": "MasterData/$(version)/*" } + ] } + """); + Write("DEEs/ProcessRules/2.59.0/After/After.cs", ProcessRuleValidationTests.Source("", ns: "_2_59_0.After")); + Write("DEEs/ProcessRules/2.59.0/Before/Before.cs", ProcessRuleValidationTests.Source("", ns: "_2_59_0.Before")); + var findings = Scan(); + Assert.Contains(findings, f => f.Rule == "ProcessRulePlacement" && f.Description.Contains("packaged as")); + Assert.Contains(findings, f => f.Rule == "ProcessRulePlacement" && f.Description.Contains("wrong side of MasterData")); + } + + [Theory] + [InlineData("ACreator.cs", "BConsumer.cs", false)] + [InlineData("BCreator.cs", "AConsumer.cs", true)] + public void SameVersionSiblingDependency_UsesManifestAndAlphabeticalOrder(string creator, string consumer, bool expected) + { + Write("DEEs/ProcessRules/2.59.0/Before/" + creator, ProcessRuleValidationTests.Source("IEntityType entity = provider.GetService(); entity.Name = \"CustomNewEntity\"; if (!entity.ObjectExists()) entity.Create();", ns: "_2_59_0.Before")); + var consumerFile = Write("DEEs/ProcessRules/2.59.0/Before/" + consumer, ProcessRuleValidationTests.Source("var entity = service.LoadEntityType(\"CustomNewEntity\");", ns: "_2_59_0.Before")); + Assert.Equal(expected, Scan([consumerFile]).Any(f => f.Rule == "ProcessRuleDependencyOrder")); + } + + [Fact] + public void ExistingTableWithChangedProperty_ProducesSchemaHazard() + { + WriteRule(TableWrite); + Write("MasterData/2.58.0/Schema.json", """{ "SmartTableProperties": { "1": { "SmartTable": "CustomTarget", "Name": "Value", "ScalarType": "String" } } }"""); + Write("MasterData/2.59.0/Schema.json", """{ "SmartTableProperties": { "1": { "SmartTable": "CustomTarget", "Name": "Value", "ScalarType": "Integer" } } }"""); + Assert.Contains(Scan(), f => f.Rule == "ProcessRuleTableMigration"); + } + + [Fact] + public void SchemaIndexIsScopedToPackage_AndReloadedEveryScan() + { + WriteRule(TableWrite); + var otherPackage = Path.Combine(_root, "Cmf.Other.Data"); + Directory.CreateDirectory(Path.Combine(otherPackage, "MasterData", "2.59.0")); + File.WriteAllText(Path.Combine(otherPackage, "cmfpackage.json"), """{ "packageType": "Data", "contentToPack": [{ "contentType": "MasterData", "source": "MasterData/$(version)/*" }] }"""); + File.WriteAllText(Path.Combine(otherPackage, "MasterData", "2.59.0", "Schema.json"), """{ "SmartTable": { "1": { "Name": "CustomTarget" } } }"""); + Assert.DoesNotContain(Scan(), f => f.Rule == "ProcessRuleTableMigration"); + Write("MasterData/2.59.0/Schema.json", """{ "SmartTable": { "1": { "Name": "CustomTarget" } } }"""); + Assert.Contains(Scan(), f => f.Rule == "ProcessRuleTableMigration"); + } + + [Fact] + public void CustomManifestSource_IsClassifiedWithoutCmfFolderAssumptions() + { + File.WriteAllText(Path.Combine(Package, "cmfpackage.json"), """{ "packageType": "Data", "contentToPack": [{ "contentType": "ProcessRulesPre", "source": "Upgrade/$(version)/*" }] }"""); + var file = Write("Upgrade/2.59.0/Sample.cs", ProcessRuleValidationTests.Source(Mutation, ns: "_2_59_0.Before")); + var info = ProcessRuleRepositoryContext.Build(_root, [file]).GetInfo(file); + Assert.NotNull(info); + Assert.Equal("Before", info.Phase); + Assert.Equal("2.59.0", info.Version); + Assert.Equal("ProcessRulesPre", info.ContentType); + Assert.Single(Scan(), f => f.Rule == "ProcessRuleUnguardedMutation"); + } +} diff --git a/cmf-codereview/tests/ProcessRuleValidationTests.cs b/cmf-codereview/tests/ProcessRuleValidationTests.cs new file mode 100644 index 0000000..f855962 --- /dev/null +++ b/cmf-codereview/tests/ProcessRuleValidationTests.cs @@ -0,0 +1,369 @@ +using System.Collections.Concurrent; +using Cmf.Cli.Plugin.CodeReview; +using Cmf.Cli.Plugin.CodeReview.Enums; +using Cmf.Cli.Plugin.CodeReview.Models; +using Cmf.Cli.Plugin.CodeReview.Rules; +using Xunit; + +namespace codereview.tests; + +public class ProcessRuleValidationTests +{ + internal static string Source(string body, string condition = "return true;", string extra = "", string ns = "EntityTypesAttributes._2_59_0") => $$""" + namespace Cmf.Custom.Actions.ProcessRules.{{ns}}; + public class Sample : DeeDevBase + { + const string AttributeName = "IsShared"; + public override bool DeeTestCondition(Dictionary Input) { {{condition}} } + public override Dictionary DeeActionCode(Dictionary Input) + { + {{body}} + return Input; + } + {{extra}} + } + """; + + private static List Review(string body, string condition = "return true;", string path = "EntityTypesAttributes/2.59.0/Sample.cs", string extra = "") + { + var source = Source(body, condition, extra); + var context = RuleFileContext.Build(source.Split('\n'), "Cmf.Custom.Data/DEEs/ProcessRules/" + path, + new MethodCallGraph(), new ConcurrentDictionary(), TestEntities.Catalog); + return new ProcessRuleValidationRule().FindMatches(context).ToList(); + } + + private const string AttributeSetup = """ + IEntityType entityType = service.LoadEntityType("Container"); + IEntityTypePropertyCollection properties = provider.GetService(); + """; + private const string AttributeCreate = "properties.Add(service.CreateAttribute(AttributeName, \"Is Shared\", scalarType));"; + private const string AttributeUpdate = "service.UpdateEntityTypeProperties(entityType, properties);"; + + [Fact] + public void ConditionOnlyGuard_ReportsSkippedConditionAndCriticalUnguardedMutation() + { + var matches = Review(AttributeSetup + AttributeCreate + AttributeUpdate, + "var entityType = service.LoadEntityType(\"Container\"); return !entityType.HasProperty(AttributeName);"); + Assert.Contains(matches, m => m.RuleName == "ProcessRuleCondition" && m.Severity == Severity.High); + Assert.Contains(matches, m => m.RuleName == "ProcessRuleUnguardedMutation" && m.Severity == Severity.Critical); + } + + [Theory] + [InlineData("if (!entityType.HasProperty(AttributeName)) {", "}")] + [InlineData("if (entityType.HasProperty(AttributeName)) return Input;", "")] + [InlineData("if (entityType.HasProperty(AttributeName)) {} else {", "}")] + [InlineData("if (entityType.HasProperty(AttributeName) == false) {", "}")] + [InlineData("if (!entityType.TryGetEntityTypeProperty(AttributeName, out var existing)) {", "}")] + [InlineData("if (!entityType.HasProperty(\"IsShared\") && enabled) {", "}")] + public void AttributeCreation_RecognizesTargetSpecificGuards(string before, string after) + { + Assert.Empty(Review(AttributeSetup + before + AttributeCreate + after + AttributeUpdate)); + } + + [Theory] + [InlineData("if (!entityType.HasProperty(\"Other\")) {")] + [InlineData("if (entityType.HasProperty(AttributeName)) {")] + [InlineData("if (!entityType.HasProperty(AttributeName) || enabled) {")] + [InlineData("if (!otherType.HasProperty(AttributeName)) {")] + public void WrongPropertyReceiverOrBooleanBranch_DoesNotSatisfyGuard(string before) + { + var matches = Review(AttributeSetup + before + AttributeCreate + "}" + AttributeUpdate); + Assert.Contains(matches, m => m.RuleName == "ProcessRuleUnguardedMutation"); + } + + [Fact] + public void ConditionalEarlyReturn_DoesNotProveAttributeIsMissing() + { + var matches = Review(AttributeSetup + "if (entityType.HasProperty(AttributeName)) { if (enabled) return Input; }" + AttributeCreate + AttributeUpdate); + Assert.Contains(matches, m => m.RuleName == "ProcessRuleUnguardedMutation"); + } + + [Fact] + public void SchemaAfterConditionalReturn_DoesNotProveRegenerationOnEveryPath() + { + Assert.Contains(Review(AttributeSetup + RawUpdate + "if (enabled) return Input; entityType.GenerateSchema();"), + m => m.RuleName == "ProcessRuleSchemaRegeneration"); + } + + [Fact] + public void ChangedTargetName_InvalidatesAnEarlierExistenceGuard() + { + Assert.Contains(Review("IAction target = provider.GetService(); if (!target.ObjectExists()) { target.Name = \"Other\"; target.Create(); }"), + m => m.RuleName == "ProcessRuleUnguardedMutation"); + } + + [Fact] + public void CreationRepeatedUnderOneAbsenceGuard_IsNotIdempotent() + { + Assert.Contains(Review("IAction target = provider.GetService(); if (!target.ObjectExists()) { target.Create(); target.Create(); }"), + m => m.RuleName == "ProcessRuleUnguardedMutation"); + } + + [Fact] + public void NontrivialConditionWithoutMatchingGuard_DoesNotEscalateMutationToCritical() + { + var matches = Review(AttributeSetup + AttributeCreate + AttributeUpdate, "LogInfo(); return true;"); + Assert.Contains(matches, m => m.RuleName == "ProcessRuleUnguardedMutation" && m.Severity == Severity.High); + } + + [Fact] + public void Termination_RequiresBothExistenceAndNotAlreadyTerminatedState() + { + Assert.Contains(Review("IRule target = provider.GetService(); if (target.ObjectExists()) target.Terminate();"), + m => m.RuleName == "ProcessRuleTerminationState"); + Assert.Empty(Review("IRule target = provider.GetService(); if (!target.ObjectExists()) return Input; if (target.UniversalState == UniversalState.Terminated) return Input; target.Terminate();")); + } + + [Fact] + public void ExternalGuardHelper_EmitsReviewHintInsteadOfProvingSafety() + { + var matches = Review(AttributeSetup + "if (service.CanCreate(entityType, AttributeName)) {" + AttributeCreate + "}" + AttributeUpdate); + Assert.Contains(matches, m => m.RuleName == "ProcessRuleUnresolvedGuard" && m.Severity == Severity.Medium); + Assert.DoesNotContain(matches, m => m.RuleName == "ProcessRuleUnguardedMutation"); + } + + [Fact] + public void ReachableLocalHelper_WithInternalGuard_IsAccepted() + { + Assert.Empty(Review(AttributeSetup + """ + void AddMissing() + { + if (!entityType.HasProperty(AttributeName)) + properties.Add(service.CreateAttribute(AttributeName, "Is Shared", scalarType)); + } + AddMissing(); + """ + AttributeUpdate)); + } + + [Fact] + public void GuardAtLocalHelperCallSite_ProtectsItsMutation() + { + Assert.Empty(Review(AttributeSetup + """ + void AddMissing() { properties.Add(service.CreateAttribute(AttributeName, "Is Shared", scalarType)); } + if (!entityType.HasProperty(AttributeName)) AddMissing(); + """ + AttributeUpdate)); + } + + [Fact] + public void ParameterizedLocalHelper_UsesGuardsForActualCallerArguments() + { + Assert.Empty(Review(AttributeSetup + """ + void AddMissing(IEntityType target, string name) + { + properties.Add(service.CreateAttribute(name, "Is Shared", scalarType)); + } + if (!entityType.HasProperty(AttributeName)) AddMissing(entityType, AttributeName); + """ + AttributeUpdate)); + } + + [Fact] + public void OneUnguardedLocalHelperCall_DoesNotBorrowOtherCallersGuard() + { + var matches = Review(AttributeSetup + """ + void AddMissing() { properties.Add(service.CreateAttribute(AttributeName, "Is Shared", scalarType)); } + if (!entityType.HasProperty(AttributeName)) AddMissing(); + AddMissing(); + """ + AttributeUpdate); + Assert.Contains(matches, m => m.RuleName == "ProcessRuleUnguardedMutation"); + } + + [Fact] + public void UnusedLocalHelperAndUnrelatedMethods_AreNotDeploymentOperations() + { + Assert.Empty(Review(AttributeSetup + "void Unused() { " + AttributeCreate + " }", + extra: "void Other() { entityType.Create(); }") ); + } + + [Fact] + public void ExpressionBodiedTrueCondition_IsAccepted() + { + var source = Source("").Replace("{ return true; }", "=> true;"); + var context = RuleFileContext.Build(source.Split('\n'), "Cmf.Custom.Data/DEEs/ProcessRules/EntityTypesAttributes/2.59.0/Sample.cs", + new MethodCallGraph(), new(), TestEntities.Catalog); + Assert.Empty(new ProcessRuleValidationRule().FindMatches(context)); + } + + [Fact] + public void RuntimeDee_DoesNotReceiveProcessRuleDiagnostics() + { + var source = Source(AttributeSetup + AttributeCreate + AttributeUpdate, "return false;"); + var context = RuleFileContext.Build(source.Split('\n'), "Cmf.Custom.Data/DEEs/Materials/Sample.cs", new MethodCallGraph(), new(), TestEntities.Catalog); + Assert.Empty(new ProcessRuleValidationRule().FindMatches(context)); + } + + private const string RawUpdate = """ + orchestration.FullUpdateEntityType(new FullUpdateEntityTypeInput + { + EntityType = entityType, + EntityTypePropertiesToAddOrUpdate = properties + }); + """; + + [Theory] + [InlineData("entityType.GenerateSchema();", false)] + [InlineData("orchestration.GenerateEntityTypeDBSchema(new GenerateEntityTypeDBSchemaInput { EntityType = entityType });", false)] + [InlineData("otherType.GenerateSchema();", true)] + [InlineData("if (enabled) entityType.GenerateSchema();", true)] + [InlineData("", true)] + public void RawPropertyUpdate_RequiresSchemaForSameEntityOnSamePath(string after, bool expected) + { + var matches = Review(AttributeSetup + RawUpdate + after); + Assert.Equal(expected, matches.Any(m => m.RuleName == "ProcessRuleSchemaRegeneration")); + } + + [Fact] + public void SchemaBeforeUpdate_DoesNotSatisfyRegeneration() + { + Assert.Contains(Review(AttributeSetup + "entityType.GenerateSchema();" + RawUpdate), m => m.RuleName == "ProcessRuleSchemaRegeneration"); + } + + [Fact] + public void ManualPropertyCollectionCreate_RecognizesGuardAndSchema() + { + Assert.Empty(Review(AttributeSetup + """ + if (!entityType.HasProperty(AttributeName)) + { + IEntityTypeProperty property = provider.GetService(); + property.Name = AttributeName; + property.EntityTypeId = entityType.Id; + properties.Add(property); + properties.Create(); + entityType.GenerateSchema(); + } + """)); + } + + private const string Removal = """ + if (entityType.TryGetEntityTypeProperty(AttributeName, out IEntityTypeProperty existing)) + properties.Add(existing); + orchestration.FullUpdateEntityType(new FullUpdateEntityTypeInput + { + EntityType = entityType, + EntityTypePropertiesToRemoveOrTerminate = properties + }); + entityType.GenerateSchema(); + """; + + [Theory] + [InlineData("materials.RemoveAttributes(new Collection { AttributeName });", false)] + [InlineData("materials.RemoveAttributes(new Collection { \"Other\" });", true)] + [InlineData("if (enabled) materials.RemoveAttributes(new Collection { AttributeName });", true)] + [InlineData("", true)] + public void AttributeRemoval_RequiresMatchingCleanupBeforeRemoval(string cleanup, bool expected) + { + var matches = Review(AttributeSetup + cleanup + Removal); + Assert.Equal(expected, matches.Any(m => m.RuleName == "ProcessRuleAttributeRemoval")); + Assert.DoesNotContain(matches, m => m.RuleName == "ProcessRuleUnguardedMutation"); + } + + [Fact] + public void GuardedRemoveAndRecreate_DoesNotRequireAnAdditionalAbsenceCheck() + { + Assert.Empty(Review(AttributeSetup + "materials.RemoveAttributes(new Collection { AttributeName });" + Removal + AttributeCreate + AttributeUpdate)); + } + + [Fact] + public void OptionalRemoval_DoesNotProtectUnconditionalRecreation() + { + var body = AttributeSetup + "if (enabled) {" + Removal + "}" + AttributeCreate + AttributeUpdate; + Assert.Contains(Review(body), m => m.RuleName == "ProcessRuleUnguardedMutation"); + } + + [Fact] + public void RepairCollectionCountGuard_IsAccepted() + { + var removal = Removal.Replace("orchestration.FullUpdateEntityType", "if (properties.Count > 0) orchestration.FullUpdateEntityType"); + Assert.DoesNotContain(Review(AttributeSetup + removal + AttributeCreate + AttributeUpdate), m => m.RuleName == "ProcessRuleUnguardedMutation"); + } + + [Fact] + public void CleanupWithSwallowedFailure_IsNotProofOfClearedValues() + { + Assert.Contains(Review(AttributeSetup + "try { materials.RemoveAttributes(new Collection { AttributeName }); } catch { }" + Removal), + m => m.RuleName == "ProcessRuleAttributeRemoval"); + } + + [Fact] + public void ParameterizedHelperInternalGuard_MustMatchCollectionUpdateTarget() + { + Assert.Empty(Review(AttributeSetup + """ + void AddMissing(IEntityType target, string name) + { + if (!target.HasProperty(name)) properties.Add(service.CreateAttribute(name, "Is Shared", scalarType)); + } + AddMissing(entityType, AttributeName); + """ + AttributeUpdate)); + Assert.Contains(Review(AttributeSetup + """ + void AddMissing(IEntityType target, string name) + { + if (!target.HasProperty(name)) properties.Add(service.CreateAttribute(name, "Is Shared", scalarType)); + } + AddMissing(otherType, AttributeName); + """ + AttributeUpdate), m => m.RuleName == "ProcessRuleUnguardedMutation"); + } + + [Fact] + public void CleanupAfterRemoval_IsCriticalOrderingViolation() + { + Assert.Contains(Review(AttributeSetup + Removal + "materials.RemoveAttributes(new Collection { AttributeName });"), + m => m.RuleName == "ProcessRuleAttributeRemoval" && m.Severity == Severity.Critical); + } + + [Fact] + public void CleanupUsingNamedAttributeList_IsRecognized() + { + Assert.DoesNotContain(Review(AttributeSetup + "var attributes = new Collection { AttributeName }; materials.RemoveAttributes(attributes);" + Removal), + m => m.RuleName == "ProcessRuleAttributeRemoval"); + } + + [Theory] + [InlineData("IAction", "Delete", "if (target.ObjectExists()) {", "}", false)] + [InlineData("IAction", "Create", "if (!target.ObjectExists()) {", "}", false)] + [InlineData("IConfig", "Delete", "if (!target.TryLoad(path)) return Input;", "", false)] + [InlineData("IRule", "Terminate", "if (target.ObjectExists() && target.UniversalState != UniversalState.Terminated) {", "}", false)] + [InlineData("IRule", "Terminate", "if (target.UniversalState == UniversalState.Terminated) return Input;", "", true)] + [InlineData("IAction", "Delete", "if (other.ObjectExists()) {", "}", true)] + [InlineData("IConfig", "Save", "", "", true)] + public void StateMutations_RequireApplicableGuard(string type, string method, string before, string after, bool expected) + { + var matches = Review($"{type} target = provider.GetService<{type}>(); {before} target.{method}(); {after}"); + Assert.Equal(expected, matches.Any(m => m.RuleName == "ProcessRuleUnguardedMutation")); + } + + [Fact] + public void RemoveActionInput_MatchesItsActionGuard() + { + var body = "IAction target = provider.GetService(); if (target.ObjectExists()) orchestration.RemoveAction(new RemoveActionInput { Action = target });"; + Assert.Empty(Review(body)); + Assert.Contains(Review(body.Replace("if (target.ObjectExists())", "")), m => m.RuleName == "ProcessRuleUnguardedMutation"); + } + + [Fact] + public void AttributesUnderGeneralPhase_AreFlagged() + { + Assert.Contains(Review(AttributeSetup + "if (!entityType.HasProperty(AttributeName)) {" + AttributeCreate + "}" + AttributeUpdate, + path: "2.59.0/After/Sample.cs"), m => m.RuleName == "ProcessRulePlacement"); + } + + [Fact] + public void MigrationUnderServices_ReportsRecurringExecution() + { + Assert.Contains(Review(AttributeSetup + "if (!entityType.HasProperty(AttributeName)) {" + AttributeCreate + "}" + AttributeUpdate, + path: "Services/Sample.cs"), m => m.RuleName == "ProcessRuleRecurringMigration" && m.Severity == Severity.Medium); + } + + [Theory] + [InlineData("EntityTypesAttributes/2.59.0/Sample.cs", "EntityTypesAttributes", "2.59.0")] + [InlineData("EntityTypes/2.59.0/Sample.cs", "EntityTypes", "2.59.0")] + [InlineData("2.59.0/Before/Sample.cs", "Before", "2.59.0")] + [InlineData("2.59.0/After/Sample.cs", "After", "2.59.0")] + [InlineData("Services/Sample.cs", "Services", null)] + [InlineData("Sample.cs", "Unknown", null)] + public void PathClassification_CoversAllDeploymentCategories(string tail, string phase, string? version) + { + var result = ProcessRuleRepositoryContext.ClassifyPath("Cmf.Semi.Data\\DEEs\\ProcessRules\\" + tail.Replace('/', '\\')); + Assert.NotNull(result); + Assert.Equal(phase, result.Phase); + Assert.Equal(version, result.Version); + } +}