Added rule MWAIS0010 to keep canonical JSON options frozen and self-contained

app/MindWork AI Studio/Assistants/VisualBriefing/VisualBriefingJson.cs

@@ -12,6 +12,9 @@ namespace AIStudio.Assistants.VisualBriefing;
 /// that never change, persistence wants output that stays readable as the app evolves. One shared
 /// configuration cannot serve both: improving the readability of stored files would rewrite the very
 /// bytes that older briefings were hashed with, and every one of them would fail its integrity check.
+/// For the same reason both configurations are written out in full instead of sharing a factory, which
+/// is what <see cref="CanonicalJsonConfigurationAttribute"/> and the rule MWAIS0010 enforce: a shared
+/// factory lets a change intended for the persistence side reach the hashed side unnoticed.
 /// </remarks>
 internal static class VisualBriefingJson
 {
@@ -27,45 +30,35 @@ internal static class VisualBriefingJson
     /// surfaces as a failed integrity check rather than as a build error. This is why enums stay numeric
     /// here even though the persisted manifest writes their member names.
     /// </remarks>
-    internal static JsonSerializerOptions Canonical { get; } = Create(writeIndented: false, enumsAsText: false);
+    [CanonicalJsonConfiguration]
+    internal static JsonSerializerOptions Canonical { get; } = new()
+    {
+        PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
+        PropertyNameCaseInsensitive = false,
+        WriteIndented = false,
+        Encoder = JavaScriptEncoder.Default,
+        UnmappedMemberHandling = JsonUnmappedMemberHandling.Disallow,
+    };
 
     /// <summary>
     /// Gets the options for files that are read back by name rather than by hash.
     /// </summary>
     /// <remarks>
     /// These options are free to evolve, because nothing hashes their output. They write the briefing
-    /// manifest and the diagnostics clipboard text, where readable enum names are worth having.
-    /// </remarks>
-    internal static JsonSerializerOptions Persistence { get; } = Create(writeIndented: true, enumsAsText: true);
-
-    /// <summary>
-    /// Creates the shared JSON configuration.
-    /// </summary>
-    /// <remarks>
-    /// Wherever the output is not hashed, enums are written as their member names instead of numbers:
-    /// stored briefings outlive many releases, so a numeric value would silently change meaning as soon
-    /// as somebody inserts or reorders an enum member. Most visual briefing enums carry the converter as
-    /// an attribute already, which applies to both configurations; this option only covers the ones
+    /// manifest and the diagnostics clipboard text, where readable enum names are worth having: stored
+    /// briefings outlive many releases, so a numeric value would silently change meaning as soon as
+    /// somebody inserts or reorders an enum member. Most visual briefing enums carry the converter as an
+    /// attribute already, which applies to both configurations; the converter below only covers the ones
     /// defined outside the feature, such as the target language and the audience enums. Reading accepts
     /// numbers as well, so manifests written before this distinction existed keep loading.
     /// </remarks>
-    /// <param name="writeIndented">Whether serialized JSON should be indented.</param>
-    /// <param name="enumsAsText">Whether enums without their own converter are written as member names.</param>
-    /// <returns>The configured serializer options.</returns>
-    private static JsonSerializerOptions Create(bool writeIndented, bool enumsAsText)
+    internal static JsonSerializerOptions Persistence { get; } = new()
     {
-        var options = new JsonSerializerOptions
-        {
-            PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
-            PropertyNameCaseInsensitive = false,
-            WriteIndented = writeIndented,
-            Encoder = JavaScriptEncoder.Default,
-            UnmappedMemberHandling = JsonUnmappedMemberHandling.Disallow,
-        };
-
-        if (enumsAsText)
-            options.Converters.Add(new JsonStringEnumConverter());
-
-        return options;
-    }
+        PropertyNamingPolicy = JsonNamingPolicy.CamelCase,
+        PropertyNameCaseInsensitive = false,
+        WriteIndented = true,
+        Encoder = JavaScriptEncoder.Default,
+        UnmappedMemberHandling = JsonUnmappedMemberHandling.Disallow,
+        Converters = { new JsonStringEnumConverter() },
+    };
 }

app/MindWork AI Studio/Tools/CanonicalJsonConfigurationAttribute.cs

@@ -0,0 +1,15 @@
+namespace AIStudio.Tools;
+
+/// <summary>
+/// Marks JSON serializer options whose exact byte output is hashed into stored data.
+/// </summary>
+/// <remarks>
+/// Options carrying this attribute are frozen: changing how they serialize changes every hash ever
+/// computed with them, which turns previously valid stored data into data that fails its integrity
+/// check. Because that failure looks like corruption rather than like a code change, the rule
+/// MWAIS0010 requires such options to be written out in full at their own declaration and to carry no
+/// converters. Sharing a factory with non-hashed options is what allows a change meant for one of them
+/// to reach the other unnoticed.
+/// </remarks>
+[AttributeUsage(AttributeTargets.Property | AttributeTargets.Field)]
+public sealed class CanonicalJsonConfigurationAttribute : Attribute;

app/SourceCodeRules/SourceCodeRules/AnalyzerReleases.Shipped.md

@@ -12,4 +12,5 @@
  MWAIS0006 | Style    | Error    | SwitchExpressionMethodAnalyzer 
  MWAIS0007 | Usage    | Error    | EmptyStringAnalyzer            
  MWAIS0008 | Naming   | Error    | LocalConstantsAnalyzer         
- MWAIS0009 | Usage    | Error    | StaticServiceProviderCacheAnalyzer
+ MWAIS0009 | Usage    | Error    | StaticServiceProviderCacheAnalyzer
+ MWAIS0010 | Usage    | Error    | CanonicalJsonConfigurationAnalyzer

app/SourceCodeRules/SourceCodeRules/AnalyzerReleases.Unshipped.md

@@ -1,7 +1,8 @@
 ### New Rules
 
- Rule ID | Category | Severity | Notes 
----------|----------|----------|-------
+ Rule ID   | Category | Severity | Notes                                
+-----------|----------|----------|--------------------------------------
+    
 
 
 ### Changed Rules

app/SourceCodeRules/SourceCodeRules/Identifier.cs

@@ -11,4 +11,5 @@ public static class Identifier
     public const string EMPTY_STRING_ANALYZER = $"{Tools.ID_PREFIX}0007";
     public const string LOCAL_CONSTANTS_ANALYZER = $"{Tools.ID_PREFIX}0008";
     public const string STATIC_SERVICE_PROVIDER_CACHE_ANALYZER = $"{Tools.ID_PREFIX}0009";
+    public const string CANONICAL_JSON_CONFIGURATION_ANALYZER = $"{Tools.ID_PREFIX}0010";
 }

app/SourceCodeRules/SourceCodeRules/UsageAnalyzers/CanonicalJsonConfigurationAnalyzer.cs

@@ -0,0 +1,140 @@
+using System.Collections.Immutable;
+using System.Linq;
+
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis.Diagnostics;
+
+namespace SourceCodeRules.UsageAnalyzers;
+
+#pragma warning disable RS1038
+[DiagnosticAnalyzer(LanguageNames.CSharp)]
+#pragma warning restore RS1038
+public sealed class CanonicalJsonConfigurationAnalyzer : DiagnosticAnalyzer
+{
+    private const string DIAGNOSTIC_ID = Identifier.CANONICAL_JSON_CONFIGURATION_ANALYZER;
+
+    private const string ATTRIBUTE_NAME = "CanonicalJsonConfigurationAttribute";
+
+    private const string CONVERTERS = "Converters";
+
+    private const string TITLE = "Canonical JSON options must stay frozen and self-contained";
+
+    private const string MESSAGE_FORMAT = "{0} The byte output of these options is hashed into stored data, so any change to them makes previously stored data fail its integrity check";
+
+    private const string DESCRIPTION = "Canonical JSON options are frozen because their exact byte output is hashed into stored data. They must be initialized inline at their own declaration, must not declare converters, and must not be reconfigured afterwards, so that a change meant for other serializer options cannot reach them through a shared factory.";
+
+    private const string CATEGORY = "Usage";
+
+    private static readonly DiagnosticDescriptor RULE = new(DIAGNOSTIC_ID, TITLE, MESSAGE_FORMAT, CATEGORY, DiagnosticSeverity.Error, isEnabledByDefault: true, description: DESCRIPTION);
+
+    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => [RULE];
+
+    public override void Initialize(AnalysisContext context)
+    {
+        context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
+        context.EnableConcurrentExecution();
+        context.RegisterSyntaxNodeAction(this.AnalyzeProperty, SyntaxKind.PropertyDeclaration);
+        context.RegisterSyntaxNodeAction(this.AnalyzeField, SyntaxKind.FieldDeclaration);
+        context.RegisterSyntaxNodeAction(AnalyzeMemberAccess, SyntaxKind.SimpleMemberAccessExpression);
+        context.RegisterSyntaxNodeAction(this.AnalyzeAssignment, SyntaxKind.SimpleAssignmentExpression);
+    }
+
+    private void AnalyzeProperty(SyntaxNodeAnalysisContext context)
+    {
+        var declaration = (PropertyDeclarationSyntax)context.Node;
+        if (context.SemanticModel.GetDeclaredSymbol(declaration) is not { } symbol || !IsMarked(symbol))
+            return;
+
+        AnalyzeInitializer(context, declaration.Initializer?.Value, declaration.Identifier.GetLocation());
+    }
+
+    private void AnalyzeField(SyntaxNodeAnalysisContext context)
+    {
+        var declaration = (FieldDeclarationSyntax)context.Node;
+        foreach (var variable in declaration.Declaration.Variables)
+        {
+            if (context.SemanticModel.GetDeclaredSymbol(variable) is not { } symbol || !IsMarked(symbol))
+                continue;
+
+            AnalyzeInitializer(context, variable.Initializer?.Value, variable.Identifier.GetLocation());
+        }
+    }
+
+    /// <summary>
+    /// Requires the complete configuration to be visible at the declaration itself.
+    /// </summary>
+    private static void AnalyzeInitializer(SyntaxNodeAnalysisContext context, ExpressionSyntax? initializer, Location location)
+    {
+        if (initializer is null)
+        {
+            context.ReportDiagnostic(Diagnostic.Create(RULE, location, "Canonical JSON options must be initialized where they are declared."));
+            return;
+        }
+
+        if (initializer is not ObjectCreationExpressionSyntax and not ImplicitObjectCreationExpressionSyntax)
+        {
+            context.ReportDiagnostic(Diagnostic.Create(RULE, initializer.GetLocation(), "Canonical JSON options must be created inline instead of by a helper, so that every setting is visible here and cannot be changed through a shared factory."));
+            return;
+        }
+
+        var settings = initializer switch
+        {
+            ObjectCreationExpressionSyntax objectCreation => objectCreation.Initializer,
+            ImplicitObjectCreationExpressionSyntax implicitCreation => implicitCreation.Initializer,
+
+            _ => null,
+        };
+
+        if (settings is null)
+            return;
+
+        foreach (var expression in settings.Expressions)
+        {
+            var name = expression switch
+            {
+                AssignmentExpressionSyntax { Left: IdentifierNameSyntax identifier } => identifier.Identifier.Text,
+
+                _ => null,
+            };
+
+            if (name == CONVERTERS)
+                context.ReportDiagnostic(Diagnostic.Create(RULE, expression.GetLocation(), "Canonical JSON options must not declare converters."));
+        }
+    }
+
+    /// <summary>
+    /// Reports reaching for the converter collection of already declared canonical options.
+    /// </summary>
+    private static void AnalyzeMemberAccess(SyntaxNodeAnalysisContext context)
+    {
+        var memberAccess = (MemberAccessExpressionSyntax)context.Node;
+        if (memberAccess.Name.Identifier.Text != CONVERTERS)
+            return;
+
+        if (!IsMarked(context.SemanticModel.GetSymbolInfo(memberAccess.Expression).Symbol))
+            return;
+
+        context.ReportDiagnostic(Diagnostic.Create(RULE, memberAccess.GetLocation(), "Canonical JSON options must not gain converters after they were declared."));
+    }
+
+    /// <summary>
+    /// Reports assigning any setting of already declared canonical options.
+    /// </summary>
+    private void AnalyzeAssignment(SyntaxNodeAnalysisContext context)
+    {
+        var assignment = (AssignmentExpressionSyntax)context.Node;
+        if (assignment.Left is not MemberAccessExpressionSyntax memberAccess)
+            return;
+
+        if (!IsMarked(context.SemanticModel.GetSymbolInfo(memberAccess.Expression).Symbol))
+            return;
+
+        context.ReportDiagnostic(Diagnostic.Create(RULE, assignment.GetLocation(), "Canonical JSON options must not be reconfigured after they were declared."));
+    }
+
+    private static bool IsMarked(ISymbol? symbol) =>
+        symbol is IPropertySymbol or IFieldSymbol &&
+        symbol.GetAttributes().Any(attribute => attribute.AttributeClass?.Name == ATTRIBUTE_NAME);
+}