fix(config): let only one bag claim a placeholder's address, +3 UI defects

The first finding corrects this branch's own reasoning. When the
addressed placeholder was introduced I considered requiring the address
to match the slot it appears in and rejected it, because a legitimate
removal shifts positions. Right rejection, wrong rule: the address never
needed to match the POSITION, it needed to be claimed by only one bag.
Duplicating a section by hand — which the new banners tell users to do
for anything the wizard cannot edit — copies its placeholders too, and
both copies resolved to the same stored credential. Many placeholders
inside one bag share its address and always will; two different bags
naming the same one is now a 400.

A transient GET /sources failure repointed a report: the Builder cleared
source.ref when the list came back empty, and a failed call was
indistinguishable from an empty one, so a blip converted a
registry-backed report into an inline one on save.

Format and destination ids were matched ordinally while the engine
resolves them case-insensitively — a stored "CSV" with the csv checkbox
ticked counted as two formats and saved two CSV outputs; the same on
destination types dropped the stored properties.

A 404 on PUT was reported as an unreachable engine. A report deleted from
another tab is a rejected request carrying a usable message, and mapping
it to Unavailable threw that message away and blamed the network.

Each fix verified by reverting it and confirming the new test fails, then
the full suite — not only the projects I thought I had touched.

Author: thiagoluga

CHANGELOG.md

@@ -52,6 +52,18 @@ The `NeoReports.Abstractions` contract follows SemVer strictly.
 - **An edited source property keeps its JSON kind.** Editing `90` turned it into `"90"`; only
   untouched rows were protected. The kind is carried from the stored value, never guessed from the
   text, so an all-digits account id stays a string.
+- **Two sections cannot both claim the same stored credential.** Duplicating an output or destination
+  block by hand copies its placeholder too, and both copies used to resolve to the same secret. An
+  address may now be claimed by one property bag only; many placeholders *inside* one bag still share
+  it, as they always must.
+- **A transient `GET /api/sources` failure no longer repoints a report.** The Builder cleared
+  `source.ref` when the list came back empty, and a failed call was indistinguishable from an empty
+  one — so a blip converted a registry-backed report into an inline one on save.
+- **Format and destination ids are matched case-insensitively**, the way the engine resolves them; a
+  stored `"CSV"` with `csv` ticked used to save two CSV outputs, and the same on destination types
+  dropped the stored properties.
+- **A `404` when saving an edit shows the engine's message**, not "the engine is not reachable" — a
+  report deleted from another tab is a rejected request, not a network failure.
 
 ### Added
 - **`GET /api/reports/{name}/config`** returns a config-origin report's stored document with

DECISIONS.md

@@ -1961,6 +1961,28 @@ authorization:
   had when the new text still fits it, and stays a string otherwise — the kind is carried, never
   guessed from the text, so an all-digits account id does not silently become a number.
 
+### The fourth review pass
+
+A `/code-review` after the non-string fix found four more, and the first one is worth reading as a
+correction to this ADR's own reasoning above. When the addressed placeholder was introduced I
+considered requiring an address to match the slot it appears in, and rejected it because a legitimate
+removal shifts positions. That was the right rejection of the wrong rule: the address never needed to
+match the *position*, it needed to be claimed by only **one** bag. Duplicating a section by hand —
+which the new banners tell users to do for anything the wizard cannot edit — copies its placeholders
+too, and both copies resolved to the same stored credential. Many placeholders inside one bag share
+its address and always will; two different bags naming the same one is now a 400.
+
+- **A transient `GET /sources` failure repointed a report.** The Builder cleared `source.ref` when the
+  registered-source list came back empty, and a failed call was indistinguishable from an empty one —
+  so a blip converted a registry-backed report into an inline one on save, silently changing what it
+  reads from.
+- **Format and destination ids were matched ordinally** while the engine resolves them
+  case-insensitively: a stored `"format": "CSV"` with the `csv` checkbox ticked counted as two formats
+  and saved two CSV outputs; the same on destination types dropped the stored properties.
+- **A `404` on `PUT` was reported as an unreachable engine.** A report deleted from another tab is a
+  rejected request carrying a usable message, and mapping it to *Unavailable* threw that message away
+  and blamed the network.
+
 ### Verified end to end
 
 Driven in a browser against `samples/09-web-ui-live`: a report carrying a literal password, a

src/NeoReports.Core/Configuration/ReportConfigSecrets.cs

@@ -157,8 +157,14 @@ public static string Restore(string document, string storedDocument)
         JsonObject root = ParseObject(document);
         JsonObject stored = ParseObject(storedDocument);
 
+        // An address may be claimed by at most one incoming bag. Many placeholders inside one bag
+        // share its address (accessKey and secretKey of the same destination), which is fine; two
+        // *different* bags naming the same one is not — duplicating a section by hand copies its
+        // placeholders too, and resolving both would hand a second destination a credential the
+        // editor cannot see, which is the outcome the address exists to prevent.
+        var claims = new Dictionary<string, JsonObject>(StringComparer.Ordinal);
         foreach ((JsonObject bag, string? _) in PropertyBagSlots(root))
-            RestoreMembers(bag, stored, []);
+            RestoreMembers(bag, new RestoreContext(stored, bag, claims), []);
 
         return root.ToJsonString();
     }
@@ -229,20 +235,25 @@ private static void RedactMembers(JsonObject owner, string sentinel)
         }
     }
 
-    private static void RestoreMembers(JsonObject bag, JsonObject stored, IReadOnlyList<object> path)
+    /// <param name="Stored">The whole stored document — a placeholder names the slot it came from.</param>
+    /// <param name="Bag">The incoming property bag currently being restored.</param>
+    /// <param name="Claims">Which bag has already claimed each address, so no two can share one.</param>
+    private sealed record RestoreContext(JsonObject Stored, JsonObject Bag, Dictionary<string, JsonObject> Claims);
+
+    private static void RestoreMembers(JsonObject owner, RestoreContext context, IReadOnlyList<object> path)
     {
-        foreach (string key in bag.Select(pair => pair.Key).ToArray())
-            Replace(bag, key, RestoreValue(bag[key], stored, [.. path, key]));
+        foreach (string key in owner.Select(pair => pair.Key).ToArray())
+            Replace(owner, key, RestoreValue(owner[key], context, [.. path, key]));
     }
 
     // Inside a property bag an array is ordered data, so an element's address is its index. Elements
     // go through the same RestoreValue as members, which is what makes a redacted *scalar* element
     // work — `"mirrors": ["…", "…"]` with a credential in one of them.
-    private static void RestoreElements(JsonArray array, JsonObject stored, IReadOnlyList<object> path)
+    private static void RestoreElements(JsonArray array, RestoreContext context, IReadOnlyList<object> path)
     {
         for (var i = 0; i < array.Count; i++)
         {
-            JsonNode? restored = RestoreValue(array[i], stored, [.. path, i]);
+            JsonNode? restored = RestoreValue(array[i], context, [.. path, i]);
             if (!ReferenceEquals(array[i], restored))
                 array[i] = restored;
         }
@@ -254,16 +265,18 @@ private static void RestoreElements(JsonArray array, JsonObject stored, IReadOnl
     /// children restored in place.
     /// </summary>
     /// <param name="value">The incoming value.</param>
-    /// <param name="stored">The whole stored document — a placeholder names the slot it came from.</param>
+    /// <param name="context">The stored document, the bag being restored, and the addresses claimed so far.</param>
     /// <param name="path">Where inside its bag this value sits, as member names and array indices.</param>
-    private static JsonNode? RestoreValue(JsonNode? value, JsonObject stored, IReadOnlyList<object> path)
+    private static JsonNode? RestoreValue(JsonNode? value, RestoreContext context, IReadOnlyList<object> path)
     {
         if (value is JsonValue sentinelValue
             && sentinelValue.TryGetValue(out string? text)
             && IsRedactedPlaceholder(text))
         {
             string? address = AddressOf(text);
-            if (!TryResolveStored(stored, address, path, out JsonNode? original))
+            ClaimAddress(context, address);
+
+            if (!TryResolveStored(context.Stored, address, path, out JsonNode? original))
             {
                 throw new ConfigurationException(
                     $"The property at '{Describe(address, path)}' was sent as a redacted placeholder, " +
@@ -276,11 +289,11 @@ private static void RestoreElements(JsonArray array, JsonObject stored, IReadOnl
         switch (value)
         {
             case JsonObject nested:
-                RestoreMembers(nested, stored, path);
+                RestoreMembers(nested, context, path);
                 break;
 
             case JsonArray array:
-                RestoreElements(array, stored, path);
+                RestoreElements(array, context, path);
                 break;
 
             default:
@@ -290,6 +303,20 @@ private static void RestoreElements(JsonArray array, JsonObject stored, IReadOnl
         return value;
     }
 
+    private static void ClaimAddress(RestoreContext context, string? address)
+    {
+        string claimed = address ?? SourceMember;
+        if (context.Claims.TryGetValue(claimed, out JsonObject? owner) && !ReferenceEquals(owner, context.Bag))
+        {
+            throw new ConfigurationException(
+                $"Two different sections both sent a redacted placeholder for '{claimed}'. A section copied " +
+                "from another one needs its own value — the placeholder only stands for the value of the " +
+                "section it came from.");
+        }
+
+        context.Claims[claimed] = context.Bag;
+    }
+
     /// <summary>
     /// Follows a placeholder's address to its stored property bag and then <paramref name="path"/>
     /// inside it. Returns <c>false</c> when any step is missing; a stored JSON <c>null</c> resolves

src/UI/NeoReports.UI/Pages/Builder.razor

@@ -93,6 +93,9 @@
     private IReadOnlyList<string> _engineSources = Array.Empty<string>();
     private IReadOnlyList<ApiSourceView> _registeredSources = Array.Empty<ApiSourceView>();
 
+    /// <summary>Whether the registered-source list actually loaded, as opposed to failing to.</summary>
+    private bool _sourcesLoaded;
+
     /// <summary>
     /// Set by ReportDetail's "Edit" button (<c>?edit=name</c>) — hydrates the wizard from that
     /// report instead of starting blank.
@@ -132,7 +135,13 @@
         }
 
         _engineSources = capabilities.Sources;
-        _registeredSources = await Api.TryListSourcesAsync() ?? Array.Empty<ApiSourceView>();
+
+        // null means the call failed, which is NOT the same as "this host has no registered sources".
+        // Conflating them cleared an edited report's source.ref on a transient blip and saved it back
+        // as an inline source — a silent repointing of what the report reads from.
+        IReadOnlyList<ApiSourceView>? registered = await Api.TryListSourcesAsync();
+        _sourcesLoaded = registered is not null;
+        _registeredSources = registered ?? Array.Empty<ApiSourceView>();
 
         // Loaded after the registered sources on purpose: a ref-based document names a source but
         // not its type (the type belongs to the registry, D42), and the Configure step's editor
@@ -150,7 +159,7 @@
         if (!Wizard.IsEditing && !_engineSources.Contains(Wizard.SourceType))
             Wizard.SourceType = _engineSources.Contains("sql") ? "sql" : _engineSources[0];
 
-        if (!string.IsNullOrEmpty(Wizard.SourceRef) && _registeredSources.All(s => s.Name != Wizard.SourceRef))
+        if (_sourcesLoaded && !string.IsNullOrEmpty(Wizard.SourceRef) && _registeredSources.All(s => s.Name != Wizard.SourceRef))
             Wizard.SourceRef = "";
     }
 

src/UI/NeoReports.UI/Services/BuilderConfigMapper.cs

@@ -113,7 +113,10 @@ public static bool Hydrate(BuilderState state, string document, Func<string, str
                 .Select(format => format!)
                 .ToArray();
 
-            state.Formats = formats.ToHashSet(StringComparer.Ordinal);
+            // The engine resolves format and destination ids case-insensitively, so the wizard has to
+            // as well: a stored "CSV" with the "csv" checkbox ticked otherwise counts as two formats
+            // and saves two CSV outputs.
+            state.Formats = formats.ToHashSet(StringComparer.OrdinalIgnoreCase);
 
             // The Format step is a set of checkboxes, so it cannot represent "two csv outputs" —
             // nothing stops a config document declaring them, with different writer properties each.
@@ -360,7 +363,7 @@ private static JsonArray BuildOutputs(BuilderState state, JsonArray? original)
             // entirely by properties the wizard cannot show, and must not be flattened by an edit.
             JsonObject[] stored = original?
                 .OfType<JsonObject>()
-                .Where(output => string.Equals((Get(output, "format") as JsonValue)?.ToString(), format, StringComparison.Ordinal))
+                .Where(output => string.Equals((Get(output, "format") as JsonValue)?.ToString(), format, StringComparison.OrdinalIgnoreCase))
                 .ToArray() ?? [];
 
             if (stored.Length == 0)
@@ -387,7 +390,7 @@ private static JsonArray BuildOutputs(BuilderState state, JsonArray? original)
         {
             JsonObject? first = original?.OfType<JsonObject>().FirstOrDefault();
             bool sameDestination = first is not null
-                && string.Equals((Get(first, "type") as JsonValue)?.ToString(), state.DestinationType, StringComparison.Ordinal);
+                && string.Equals((Get(first, "type") as JsonValue)?.ToString(), state.DestinationType, StringComparison.OrdinalIgnoreCase);
 
             // On a type switch the stored properties are dropped: an S3 bucket and region are not
             // configuration a local-filesystem destination should inherit.

src/UI/NeoReports.UI/Services/NeoReportsApiClient.cs

@@ -662,9 +662,13 @@ public async Task<ApiCreateResult> TryReplaceReportAsync(
             string? error = await TryReadErrorAsync(response, cancellationToken).ConfigureAwait(false);
             // A 409 here is not "name taken" (the name is this report's own) but "this report is
             // code-registered", which is a different message entirely — hence Invalid, not NameTaken.
+            // A 404 means the report was deleted from somewhere else while this wizard was open; it
+            // is a rejected request carrying a usable message, not an unreachable engine, and mapping
+            // it to Unavailable threw that message away and blamed the network instead.
             ApiCreateOutcome outcome = response.StatusCode switch
             {
-                HttpStatusCode.BadRequest or HttpStatusCode.Conflict => ApiCreateOutcome.Invalid,
+                HttpStatusCode.BadRequest or HttpStatusCode.Conflict or HttpStatusCode.NotFound =>
+                    ApiCreateOutcome.Invalid,
                 _ => ApiCreateOutcome.Unavailable,
             };
             return new ApiCreateResult(outcome, null, error);

tests/NeoReports.Core.UnitTests/ReportConfigSecretsTests.cs

@@ -500,6 +500,39 @@ public void A_section_without_a_property_bag_does_not_shift_the_addresses_after_
             .GetProperty("properties").GetProperty("accessKey").GetString().ShouldBe("KEY-BETA");
     }
 
+    [Fact]
+    public void Two_sections_cannot_both_claim_the_same_stored_credential()
+    {
+        const string stored = """{"destinations":[{"type":"s3","properties":{"accessKey":"KEY-ALPHA"}}]}""";
+        // Duplicating a section by hand copies its placeholder too. Resolving both would hand the
+        // second destination a credential the editor cannot see — the outcome the address exists to
+        // prevent, arrived at by duplication instead of by inference.
+        const string duplicated = """
+            {"destinations":[{"type":"s3","properties":{"accessKey":"${neoreports:redacted:destinations[0]}"}},
+                             {"type":"s3","properties":{"accessKey":"${neoreports:redacted:destinations[0]}"}}]}
+            """;
+
+        Should.Throw<ConfigurationException>(() => ReportConfigSecrets.Restore(duplicated, stored))
+            .Message.ShouldContain("destinations[0]");
+    }
+
+    [Fact]
+    public void Several_placeholders_inside_one_section_share_its_address_freely()
+    {
+        // The claim is per bag, not per placeholder: one destination's accessKey and secretKey both
+        // name the same slot, and always will.
+        const string stored = """
+            {"destinations":[{"type":"s3","properties":{"accessKey":"KA","secretKey":"KS","bucket":"b"}}]}
+            """;
+
+        string restored = ReportConfigSecrets.Restore(ReportConfigSecrets.Redact(stored), stored);
+
+        JsonElement properties = JsonDocument.Parse(restored).RootElement
+            .GetProperty("destinations").EnumerateArray().Single().GetProperty("properties");
+        properties.GetProperty("accessKey").GetString().ShouldBe("KA");
+        properties.GetProperty("secretKey").GetString().ShouldBe("KS");
+    }
+
     [Fact]
     public void A_placeholder_addressing_a_section_that_no_longer_exists_is_rejected()
     {

tests/NeoReports.UI.UnitTests/BuilderConfigMapperTests.cs

@@ -675,6 +675,41 @@ public void An_addressed_placeholder_on_the_connection_is_sent_back_verbatim()
             .GetProperty("connectionString").GetString().ShouldBe("${neoreports:redacted}");
     }
 
+    [Fact]
+    public void A_format_id_stored_in_a_different_case_is_not_treated_as_a_second_format()
+    {
+        var state = new BuilderState();
+        BuilderConfigMapper.Hydrate(state, """
+            {"name":"feed","source":{"type":"http"},"outputs":[{"format":"CSV","properties":{"delimiter":";"}}]}
+            """).ShouldBeTrue();
+        state.Formats.Add("csv");
+
+        using JsonDocument doc = JsonDocument.Parse(BuilderConfigMapper.ToConfigJson(state));
+
+        // The engine resolves format ids case-insensitively; matching them ordinally here made a
+        // stored "CSV" plus a ticked "csv" checkbox save two CSV outputs.
+        JsonElement[] outputs = doc.RootElement.GetProperty("outputs").EnumerateArray().ToArray();
+        outputs.Length.ShouldBe(1);
+        outputs[0].GetProperty(PropertiesMember).GetProperty("delimiter").GetString().ShouldBe(";");
+    }
+
+    [Fact]
+    public void A_destination_type_stored_in_a_different_case_keeps_its_properties()
+    {
+        var state = new BuilderState();
+        BuilderConfigMapper.Hydrate(state, """
+            {"name":"feed","source":{"type":"http"},
+             "destinations":[{"type":"S3","properties":{"bucket":"reports","path":"a.csv"}}]}
+            """).ShouldBeTrue();
+        state.DestinationType = "s3";
+
+        using JsonDocument doc = JsonDocument.Parse(BuilderConfigMapper.ToConfigJson(state));
+
+        // Ordinal matching treated this as a type change and dropped the stored bucket.
+        doc.RootElement.GetProperty("destinations").EnumerateArray().Single()
+            .GetProperty(PropertiesMember).GetProperty("bucket").GetString().ShouldBe("reports");
+    }
+
     [Fact]
     public void Switching_the_source_drops_the_stored_properties_and_the_kept_connection()
     {

tests/NeoReports.UI.UnitTests/Pages/BuilderReviewTests.cs

@@ -115,6 +115,23 @@ public void Editing_with_an_invalid_config_reports_the_engine_error_and_leaves_t
         cut.Markup.ShouldContain("Query references an unknown column.");
     }
 
+    [Fact]
+    public void Editing_a_report_deleted_elsewhere_shows_the_engines_message_not_a_network_error()
+    {
+        Wizard.IsEditing = true;
+        Wizard.EditingOriginalName = "clientsVip";
+        Api.ReplaceReport = (_, _, _) => Task.FromResult(
+            new ApiCreateResult(ApiCreateOutcome.Invalid, null, "No report named 'clientsVip' is registered."));
+
+        var cut = RenderReview();
+        cut.FindAll("button").First(b => b.TextContent.Contains("Save report")).Click();
+
+        // A 404 is a rejected request carrying a usable message — mapping it to Unavailable threw the
+        // message away and blamed the network for a report someone deleted in another tab.
+        cut.Markup.ShouldContain("No report named 'clientsVip' is registered.");
+        cut.Markup.ShouldNotContain("The engine is not reachable right now.");
+    }
+
     [Fact]
     public void Editing_saves_through_a_single_replace_call_and_never_deletes()
     {