Fix plugin installer writing values it should not (#2264, #2266, #2271)

src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-26-a-refused-build-stops-retrying-itself.md

@@ -0,0 +1,33 @@
+---
+Name: A refused build stops retrying itself
+Category: Fix
+Description: On a mesh that requires prebuilt assemblies, a type refused for lack of a bundle used to burn one automatic retry attempt it never needed — a second trip through the same refusal, for nothing. The refusal now records what it was decided under, so the automatic retry recognises it was already answered.
+Icon: Sparkle
+Order: -20260826
+---
+
+# A refused build stops retrying itself
+
+`Modules:RequirePrebuilt` parks a type instead of compiling it when no prebuilt assembly has landed
+for it — a named, bounded refusal instead of a silent Roslyn pass. The framework also carries a
+separate, general recovery: a type sitting at a build failure with nothing recorded about what it was
+tried under gets exactly one automatic retry, so a fix that ships later can still reach it without
+anyone pressing a button.
+
+The two mechanisms shared a gap. A require-prebuilt refusal never recorded what it was decided under,
+so the recovery mechanism read every refusal as "never attempted" — and drove its one automatic retry
+on a type that had, in fact, just been correctly and deliberately refused. The retry landed back on
+the same refusal a moment later, spending part of a budget meant for genuine recoveries on a refusal
+that needed none.
+
+## What changed
+
+A require-prebuilt refusal now records the inputs — the framework build, the installed modules, the
+source set — it was decided under, in the same write that settles it. The recovery mechanism compares
+against that record and correctly finds nothing to retry, because nothing about the refusal has
+changed since it was made.
+
+## What you will notice
+
+A type refused for lack of a bundle settles once and stays settled — no extra round through the
+compile pipeline, and the automatic-retry budget is left for the failures that actually need it.

src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-26-package-installs-keep-authored-content.md

@@ -0,0 +1,44 @@
+---
+Name: Package installs keep the content you actually authored
+Category: Fix
+Description: A package file with a leading byte-order mark and a dynamically-compiled content type could install a materialised value instead of the file's own content, and rewrite that node on every later install. Both are fixed at the root — the installer's authored-content re-read now tolerates exactly what the primary parse already tolerates.
+Icon: FileCheckmark
+Order: -20260826
+---
+
+# Package installs keep the content you actually authored
+
+Installing a content package (a node-repo plugin, a course, a NodeType's instances) writes what the
+file says. For most files that was always true. For one narrow but real case it was not: an instance
+file whose content type is a NodeType that has already compiled.
+
+## What was happening
+
+Once a NodeType compiles, its content type becomes a name the mesh can resolve — but only by name.
+The same short name can exist in more than one package, so the installer's own parse of such a file
+does not trust the typed value it gets back: it re-reads the file's `content` property as written and
+installs that instead, specifically so one package's compiled defaults can never leak into another
+package's node.
+
+That re-read used stricter rules than the parse that produced the typed value in the first place. A
+file that opened with a UTF-8 byte-order mark — invisible in an editor, and common enough that one
+sample package ships several instance files this way — made the re-read fail. When it failed, the
+installer fell back to the materialised value after all: the one thing this whole mechanism exists to
+avoid, logged as a warning naming exactly that risk.
+
+That materialised value could then differ from the authored file in ways that had nothing to do with
+cross-package contamination — a numeric field with a matching default, present in the file, absent
+from the re-serialised value. The next install's unchanged-check compared the stored node against the
+authored file, found a difference that was never really there, and rewrote the node again. Every
+install after that repeated the same rewrite.
+
+## What changed
+
+The installer's authored-content re-read now shares every tolerance the primary parse already
+applies — the same byte-order-mark stripping, the same comment and trailing-comma leniency — instead
+of falling back to stricter defaults. A file that parsed once now re-reads the same way, every time.
+
+## What you will notice
+
+A package install writes the node your file actually declares, and a second install of an unchanged
+package writes nothing at all.

src/MeshWeaver.Graph/Configuration/NodeTypeCompilationHelpers.cs

@@ -247,7 +247,10 @@ public static IDisposable InstallCompileWatcher(
                     // trigger is answered (bounded, no Roslyn). Same fire-and-forget UpdateOwn
                     // shape as the kickoffs below; System scope because re-settling framework
                     // state is infrastructure, not a user write.
-                    void SettleAsError(string? reason)
+                    // 🚨 #2264: the TWO call sites of this local function need DIFFERENT
+                    // FailedBuildInputs treatment, so `formedUnderLiveInputs` is not decoration —
+                    // see ApplyGateSettle's doc for why.
+                    void SettleAsError(string? reason, bool formedUnderLiveInputs)
                     {
                         using (accessService?.ImpersonateAsSystem())
                             workspace.GetMeshNodeStream().Update(curr =>
@@ -261,13 +264,8 @@ void SettleAsError(string? reason)
                                     return curr;
                                 return curr with
                                 {
-                                    Content = parkedDef with
-                                    {
-                                        CompilationStatus = CompilationStatus.Error,
-                                        CompilationError = reason
-                                            ?? parkedDef.CompilationError
-                                            ?? "Compilation is parked after a terminal failure; request a release (Compile) to retry."
-                                    }
+                                    Content = ApplyGateSettle(
+                                        parkedDef, reason, formedUnderLiveInputs, guards.ModulesHash)
                                 };
                             }).Subscribe(
                                 _ => { },
@@ -299,7 +297,10 @@ void SettleAsError(string? reason)
                             logger?.LogDebug(
                                 "Compile watcher: {HubPath} is PARKED (terminal compile failure) — " +
                                 "skipping recompile, serving cached error", hubPath);
-                            SettleAsError(parkRegistry.GetParkedError(hubPath));
+                            // The verdict being RE-SERVED is the ORIGINAL failure's, formed under
+                            // ITS inputs — stamping the live ones here would mask a genuine input
+                            // change since that failure and defeat #1793's whole recovery path.
+                            SettleAsError(parkRegistry.GetParkedError(hubPath), formedUnderLiveInputs: false);
                             return;
                         }
                     }
@@ -452,7 +453,10 @@ void DispatchOrPark()
                             registry?.OnCompileFailed(
                                 hub, hubPath, reason, deterministic: true,
                                 recipientUserId: null, sources: pendingDef?.CurrentSourceVersions, logger);
-                            SettleAsError(reason);
+                            // The refusal genuinely IS formed under the live compile inputs — stamp
+                            // them so the failed-verdict re-drive (#1793) doesn't treat this as
+                            // "never attempted" and burn a needless automatic re-drive (#2264).
+                            SettleAsError(reason, formedUnderLiveInputs: true);
                             return;
                         }
 
@@ -1943,6 +1947,54 @@ internal static MeshNode ApplyFailedVerdictRedrive(
         };
     }
 
+    /// <summary>
+    /// 🚨 THE terminal stamp of a Pending flip the compile watcher refuses to dispatch WITHOUT
+    /// running Roslyn (issue #2264) — the shared body of <c>InstallCompileWatcher</c>'s
+    /// <c>SettleAsError</c> local function, extracted so the per-call-site
+    /// <see cref="NodeTypeDefinition.FailedBuildInputs"/> decision is unit-testable without a hub.
+    ///
+    /// <para><c>SettleAsError</c> is the ONLY way a NodeType reaches <see cref="CompilationStatus.Error"/>
+    /// under the adopt-only gate (<c>Modules:RequirePrebuilt</c>, #2193 §A), which never runs a
+    /// compile — so it is the only writer of <see cref="NodeTypeDefinition.FailedBuildInputs"/> for
+    /// such a type, and the two call sites need OPPOSITE answers:</para>
+    ///
+    /// <list type="bullet">
+    ///   <item>the <b>adopt-only gate's refusal</b> (<paramref name="formedUnderLiveInputs"/> =
+    ///     <c>true</c>) is formed under the LIVE compile inputs RIGHT NOW, so stamping them is
+    ///     honest — and is what stops <see cref="HasStaleFailureVerdict"/> from reading an
+    ///     unstamped verdict as "never attempted" and driving one needless automatic re-drive
+    ///     (#1793) per refused type.</item>
+    ///   <item>the <b>parked short-circuit's re-settle</b> (<paramref name="formedUnderLiveInputs"/>
+    ///     = <c>false</c>) re-serves an ALREADY-SETTLED failure's verdict — stamping the CURRENT
+    ///     live inputs there would overwrite (or fabricate, for a type parked before this fix)
+    ///     the record of what THAT original failure was formed under, silently masking a genuine
+    ///     input change since that failure and defeating #1793's whole recovery path. So it leaves
+    ///     whatever <see cref="NodeTypeDefinition.FailedBuildInputs"/> the node already carries
+    ///     untouched — which is exactly what OMITTING the field from the <c>with</c> would do too;
+    ///     spelled out here so the two branches are visibly symmetric instead of one being a
+    ///     silent no-op.</item>
+    /// </list>
+    /// </summary>
+    /// <param name="parkedDef">The NodeType definition as currently persisted (authoritative — read
+    /// inside the <c>Update</c> lambda, never a captured snapshot).</param>
+    /// <param name="reason">The refusal/parked-error message, or <c>null</c> to keep whatever
+    /// <see cref="NodeTypeDefinition.CompilationError"/> is already recorded.</param>
+    /// <param name="formedUnderLiveInputs">Whether THIS settle is itself a decision formed under
+    /// the live compile inputs (see above).</param>
+    /// <param name="modulesHash">The installed-module fingerprint half of the compile inputs.</param>
+    internal static NodeTypeDefinition ApplyGateSettle(
+        NodeTypeDefinition parkedDef, string? reason, bool formedUnderLiveInputs, string? modulesHash) =>
+        parkedDef with
+        {
+            CompilationStatus = CompilationStatus.Error,
+            CompilationError = reason
+                ?? parkedDef.CompilationError
+                ?? "Compilation is parked after a terminal failure; request a release (Compile) to retry.",
+            FailedBuildInputs = formedUnderLiveInputs
+                ? BuildInputsToken(modulesHash, parkedDef.CurrentSourceVersions)
+                : parkedDef.FailedBuildInputs,
+        };
+
     /// <summary>
     /// THE terminal stamp of a SUCCESSFUL compile — the exact field set
     /// <see cref="RunCompile"/>'s write-back has always applied, extracted as a pure function so

src/MeshWeaver.PluginCatalog/PackageInstaller.cs

@@ -120,7 +120,7 @@ private static IObservable<InstallResult> InstallCore(
 
         var nodes = files
             .Where(f => !IsManifest(f.RelativePath))
-            .Select(f => ParseNode(parsers, partition!, sourceFolder, f, logger))
+            .Select(f => ParseNode(parsers, partition!, sourceFolder, f, logger, hub.JsonSerializerOptions))
             .Where(n => n is not null).Select(n => n!)
             .ToArray();
 
@@ -1798,7 +1798,7 @@ private static IObservable<InstallResult> InstallCode(
 
         var sourceNodes = files
             .Where(f => !IsManifest(f.RelativePath))
-            .Select(f => ParseNode(parsers, nodeTypePath, sourceFolder, f, logger))
+            .Select(f => ParseNode(parsers, nodeTypePath, sourceFolder, f, logger, hub.JsonSerializerOptions))
             .Where(n => n is not null).Select(n => n!)
             .ToArray();
 
@@ -2106,7 +2106,7 @@ private static IObservable<InstallResult> InstallNodeRepo(
             .Where(f => ModuleManifest.IsManifestPath(f.RelativePath))
             .Select(f => ModuleManifest.TryParse(f.Content, logger))
             .FirstOrDefault(m => m is not null);
-        var nodes = ParseAll(parsers, files, manifest.Id, logger);
+        var nodes = ParseAll(parsers, files, manifest.Id, logger, hub.JsonSerializerOptions);
 
         if (nodes.Length == 0)
             return Observable.Throw<InstallResult>(new InvalidOperationException(
@@ -2662,7 +2662,7 @@ private static IObservable<InstallResult> InstallNodeRepoDeltaCore(
     {
 
         var parsers = new FileFormatParserRegistry(hub.JsonSerializerOptions, hub.ServiceProvider.GetServices<IFileFormatParser>());
-        var nodes = ParseAll(parsers, changedFiles, manifest.Id, logger);
+        var nodes = ParseAll(parsers, changedFiles, manifest.Id, logger, hub.JsonSerializerOptions);
 
         if (RefuseIfStaticShadowed(hub, manifest, nodes, logger) is { } shadowed)
             return shadowed;
@@ -2834,11 +2834,12 @@ IObservable<int> Prune() =>
     /// </para>
     /// </summary>
     private static MeshNode[] ParseAll(
-        FileFormatParserRegistry parsers, IReadOnlyList<PackageFile> files, string packageId, ILogger? logger)
+        FileFormatParserRegistry parsers, IReadOnlyList<PackageFile> files, string packageId,
+        ILogger? logger, JsonSerializerOptions? options = null)
     {
         var unparsed = new List<string>();
         var nodes = files
-            .Select(f => ParseCanonical(parsers, f, logger, unparsed))
+            .Select(f => ParseCanonical(parsers, f, logger, options, unparsed))
             .Where(n => n is not null).Select(n => n!)
             .ToArray();
 
@@ -2876,7 +2877,8 @@ private static bool IsNotANodeFile(string relativePath) =>
         || ContentAssetMapper.IsContentPath(relativePath);
 
     private static MeshNode? ParseCanonical(
-        FileFormatParserRegistry parsers, PackageFile file, ILogger? logger, List<string>? unparsed = null)
+        FileFormatParserRegistry parsers, PackageFile file, ILogger? logger,
+        JsonSerializerOptions? options = null, List<string>? unparsed = null)
     {
         if (IsNotANodeFile(file.RelativePath))
             return null;
@@ -2889,7 +2891,7 @@ private static bool IsNotANodeFile(string relativePath) =>
             return null;
         }
         var (id, ns) = NodeFileMapper.FromRelativePath(file.RelativePath);
-        return AsAuthored(parsed, file, logger) with
+        return AsAuthored(parsed, file, logger, options) with
         {
             Id = id,
             Namespace = ns,
@@ -2924,23 +2926,49 @@ private static bool IsNotANodeFile(string relativePath) =>
     /// (<see cref="NodeTypeDefinition"/>, markdown, access assignments) is a real, process-unique
     /// registration rather than a name guess, so it stays typed: the installer's own ordering and
     /// compile-trigger logic reads <c>Content is NodeTypeDefinition</c>.</para>
+    ///
+    /// <para>🚨 #2266: the re-read here MUST tolerate exactly what the PRIMARY parse tolerated —
+    /// <see cref="FileFormatParserRegistry.TryParse"/> already produced <paramref name="parsed"/>
+    /// from this very <paramref name="file"/>, after stripping a leading UTF-8 BOM
+    /// (<see cref="FileFormatParserRegistry.WithoutBom"/>) and applying <paramref name="options"/>'s
+    /// comment/trailing-comma leniency. Re-parsing the RAW, un-stripped <c>file.Content</c> under
+    /// <see cref="JsonDocumentOptions"/>' strict defaults made the fallback below reachable for
+    /// every BOM'd file the primary parse tolerated — <c>samples/Graph/Data/PensionFund</c> ships
+    /// its Currency/Position/Year instances BOM'd, so every one of them silently installed the
+    /// materialised (possibly wrong-package) value instead of the authored file, which is also what
+    /// made the CHF/EUR/USD Currency nodes fail the idempotence check on a second install (#2271):
+    /// the wrongly-installed materialised value is what the unchanged-skip then compared against.</para>
     /// </summary>
     // Internal for the InstallAuthoredContentTest pin (InternalsVisibleTo).
-    internal static MeshNode AsAuthored(MeshNode parsed, PackageFile file, ILogger? logger)
+    internal static MeshNode AsAuthored(
+        MeshNode parsed, PackageFile file, ILogger? logger, JsonSerializerOptions? options = null)
     {
         if (parsed.Content is null || !parsed.Content.GetType().Assembly.IsCollectible)
             return parsed;
         try
         {
-            using var doc = JsonDocument.Parse(file.Content);
+            // Same three tolerances JsonFileParser.Parse copies off the hub's own options — this
+            // read must never be STRICTER than the parse that already succeeded on this content.
+            var documentOptions = options is null
+                ? default
+                : new JsonDocumentOptions
+                {
+                    CommentHandling = options.ReadCommentHandling,
+                    AllowTrailingCommas = options.AllowTrailingCommas,
+                    MaxDepth = options.MaxDepth,
+                };
+            using var doc = JsonDocument.Parse(
+                FileFormatParserRegistry.WithoutBom(file.Content), documentOptions);
             if (doc.RootElement.ValueKind == JsonValueKind.Object
                 && doc.RootElement.TryGetProperty("content", out var authored))
                 return parsed with { Content = authored.Clone() };
         }
         catch (JsonException)
         {
-            // Unreachable in practice — a file that produced a typed content parsed as JSON once
-            // already. Fall through rather than invent a content shape.
+            // Reachable only for content that is genuinely malformed in a way the primary parse's
+            // OWN deserialization somehow tolerated (e.g. the "content" property itself is odd
+            // enough for JsonFileParser's typed deserialize to accept but this raw re-read cannot
+            // structurally locate) — fall through rather than invent a content shape.
         }
         logger?.LogWarning(
             "[PackageInstaller] {Path} materialised the runtime-compiled content type {Type}, and the "
@@ -2973,7 +3001,8 @@ private static IObservable<int> Upsert(IMessageHub hub, MeshNode node) =>
     // Parse one package file into a node rebased under the target partition (mirrors
     // GitHubSyncService.ParseFile). The package.json manifest is filtered out before this.
     private static MeshNode? ParseNode(
-        FileFormatParserRegistry parsers, string partition, string sourceFolder, PackageFile file, ILogger? logger)
+        FileFormatParserRegistry parsers, string partition, string sourceFolder, PackageFile file,
+        ILogger? logger, JsonSerializerOptions? options = null)
     {
         var rel = FolderRelative(file.RelativePath, sourceFolder);
 
@@ -2992,7 +3021,7 @@ private static IObservable<int> Upsert(IMessageHub hub, MeshNode node) =>
 
         var (id, ns) = NodeFileMapper.FromRelativePath(rel);
         var rebasedNs = string.IsNullOrEmpty(ns) ? partition : $"{partition}/{ns}";
-        return AsAuthored(parsed, file, logger) with
+        return AsAuthored(parsed, file, logger, options) with
         {
             Id = id,
             Namespace = rebasedNs,