fix(ui): keep a ref report's connection overlay, +4 edit-flow defects

None of these five touched the secrets mechanism, which the review pass
traced end to end without breaking; all five were in what surrounds it.

A ref-based report's report-local connectionString was deleted on save.
The wizard offers no connection field for a registered source, so the
save path dropped it — but a report-local overlay is legitimate config
under D42 (report-local wins), and deleting it silently repointed the
report at the registry's connection. A stored one is kept as it arrived;
one is still never invented, since there is no field to invent it from.
The matching banner also stopped rendering for ref sources, where it
pointed at a control that is not on the page.

A failed GET .../config was indistinguishable from "not editable", so
Edit degraded to a blank create wizard with no message — inviting the
user to retype a whole report over a working one. The more useful
observation is that this is the same conflation _sourcesLoaded had just
been added to fix one function above: I fixed the instance, not the
pattern.

A corrupt STORED document was a 400 on PUT while GET .../config answered
the identical condition with a 500, blaming the caller for a file they
never sent.

validate?for= reported the report's own name as taken, putting "name
already taken" under every successful edit validation.

Changing the source mid-edit left no way forward: the old credential is
correctly dropped, but nothing said so and the save failed with a generic
compile error.

Each fix verified by reverting it and confirming the new test fails, then
the full suite: 1 156 tests green.

Author: thiagoluga

CHANGELOG.md

@@ -64,6 +64,16 @@ The `NeoReports.Abstractions` contract follows SemVer strictly.
   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.
+- **A `ref`-based report keeps its report-local connection overlay.** The wizard offers no connection
+  field for a registered source, so saving deleted any `connectionString` — silently repointing the
+  report at the registry's connection, though D42 makes a report-local overlay win.
+- **A failed configuration load says so** instead of silently becoming a blank create wizard.
+- **A corrupt stored document is a `500` on `PUT`**, matching what `GET .../config` already returns
+  for the same condition, rather than a `400` blaming the caller.
+- **`validate?for={name}` no longer reports that report's own name as taken**, which put "name already
+  taken" under every successful edit validation.
+- **Changing the source mid-edit now says the connection must be re-supplied**, rather than failing at
+  save with a generic compile error.
 
 ### Added
 - **`GET /api/reports/{name}/config`** returns a config-origin report's stored document with

DECISIONS.md

@@ -1983,6 +1983,30 @@ its address and always will; two different bags naming the same one is now a 400
   rejected request carrying a usable message, and mapping it to *Unavailable* threw that message away
   and blamed the network.
 
+### The fifth review pass
+
+None of these five touched the secrets mechanism itself, which the pass traced end to end without
+breaking; all five were in what surrounds it.
+
+- **A `ref`-based report's report-local connection overlay was deleted on save.** The wizard offers no
+  connection field for a registered source, so the save path dropped any `connectionString` — but a
+  report-local overlay is legitimate configuration under D42 (report-local wins), and deleting it
+  silently repointed the report at the registry's connection. A stored one is now kept as it arrived;
+  one is still never invented, since there is no field to invent it from. The matching "the stored
+  connection is kept" banner also stopped rendering for `ref` sources, where it pointed at a control
+  that is not on the page.
+- **A failed `GET .../config` was indistinguishable from "not editable"**, so Edit degraded to a blank
+  create wizard with no message — inviting the user to retype a whole report over a working one. This
+  is the same conflation `_sourcesLoaded` had just been added to fix one function above, which is the
+  more useful observation: the fix was applied to the instance, not to the pattern.
+- **A corrupt *stored* document was a 400 on `PUT`** while `GET .../config` answered the identical
+  condition with a 500 — blaming the caller for a file they never sent.
+- **`validate?for=` reported the report's own name as taken**, putting "name already taken" under
+  every successful edit validation.
+- **Changing the source mid-edit left no way forward**: the old credential is correctly dropped, but
+  nothing said so, and the save failed with a generic compile error. The Configure step now says what
+  has to be supplied.
+
 ### Verified end to end
 
 Driven in a browser against `samples/09-web-ui-live`: a report carrying a literal password, a

src/Integrations/NeoReports.AspNetCore/NeoReportsEndpointRouteBuilderExtensions.cs

@@ -583,6 +583,20 @@ private static async Task<IResult> ReplaceReportAsync(
             });
         }
 
+        // Checked before Restore merges the two, so a corrupt document on disk is not reported as a
+        // bad request — the same condition GET .../config already answers with a 500.
+        try
+        {
+            ReportConfigSecrets.EnsureReadable(stored);
+        }
+        catch (ConfigurationException ex)
+        {
+            return Results.Problem(
+                title: $"The stored configuration for '{name}' could not be read.",
+                detail: ex.Message,
+                statusCode: StatusCodes.Status500InternalServerError);
+        }
+
         string document = await ReadBodyAsync(http, cancellationToken).ConfigureAwait(false);
 
         ReportConfig config;
@@ -747,14 +761,18 @@ private static async Task<IResult> ValidateReportAsync(
             CompiledReport compiled = ReportConfigCompiler.Compile(substituted, rootServices);
             var columns = compiled.Schema.Columns.Select(c => c.Name).ToArray();
 
+            // Its own name is not "taken" when the dry run IS an edit of that report — reporting it
+            // as taken put "name already taken" under every successful edit validation.
             return Results.Ok(new ValidateReportResponse(
-                Valid: true, Error: null, Name: config.Name, Columns: columns, NameTaken: registry.Contains(config.Name)));
+                Valid: true, Error: null, Name: config.Name, Columns: columns,
+                NameTaken: registry.Contains(config.Name) && !string.Equals(config.Name, editingFor, StringComparison.Ordinal)));
         }
         catch (ConfigurationException ex)
         {
             return Results.Ok(new ValidateReportResponse(
                 Valid: false, Error: ex.Message, Name: name, Columns: null,
-                NameTaken: name is not null && registry.Contains(name)));
+                NameTaken: name is not null && registry.Contains(name)
+                    && !string.Equals(name, editingFor, StringComparison.Ordinal)));
         }
     }
 

src/NeoReports.Core/Configuration/ReportConfigSecrets.cs

@@ -110,6 +110,15 @@ public static string Redact(string document)
     public static bool ContainsRedactedValue(string document) =>
         PropertyBagSlots(ParseObject(document)).Any(slot => ContainsRedactedNode(slot.Bag));
 
+    /// <summary>
+    /// Throws when <paramref name="document"/> is not a readable configuration document. Lets a
+    /// caller tell a corrupt *stored* document from a bad request body before <see cref="Restore"/>
+    /// merges the two and reports both the same way.
+    /// </summary>
+    /// <param name="document">The document to check.</param>
+    /// <exception cref="ConfigurationException">Thrown when the document is missing or not a JSON object.</exception>
+    public static void EnsureReadable(string document) => ParseObject(document);
+
     /// <summary>True when <paramref name="text"/> is a redaction placeholder, addressed or not.</summary>
     /// <param name="text">The value to test.</param>
     public static bool IsRedactedPlaceholder(string? text) =>

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

@@ -28,6 +28,13 @@
             <span style="font-size:12px">Every step is prefilled from the report's stored configuration. Values the engine holds back — connection strings, API tokens — are kept as they are unless you replace them.</span>
         </Banner>
     }
+    @if (_configUnavailable)
+    {
+        <Banner Variant="danger" Icon="alert-triangle">
+            <b>Could not load that report's configuration.</b>
+            <span style="font-size:12px">Nothing was prefilled, so this is a blank new report — saving it would not edit the one you came from. Go back and try again.</span>
+        </Banner>
+    }
     @if (!Wizard.EngineAvailable)
     {
         <Banner Variant="info" Icon="info-circle">
@@ -96,6 +103,9 @@
     /// <summary>Whether the registered-source list actually loaded, as opposed to failing to.</summary>
     private bool _sourcesLoaded;
 
+    /// <summary>Set when an edit was asked for and the report's configuration could not be read.</summary>
+    private bool _configUnavailable;
+
     /// <summary>
     /// Set by ReportDetail's "Edit" button (<c>?edit=name</c>) — hydrates the wizard from that
     /// report instead of starting blank.
@@ -176,10 +186,15 @@
     {
         Wizard.Reset();
 
-        string? document = await Api.TryGetReportConfigAsync(name);
-        // No stored document means a code-registered report (or a gone one), which is not editable
-        // here at all — falls back to a blank "new report" wizard rather than a half-filled form.
-        if (document is null || !BuilderConfigMapper.Hydrate(Wizard, document, RegisteredSourceType))
+        ApiConfigResult config = await Api.TryGetReportConfigAsync(name);
+
+        // "Could not load it" is not "it is not editable" — the same conflation _sourcesLoaded fixes
+        // above. Silently degrading to a blank create wizard on a transient failure would invite the
+        // user to type a whole report over a working one.
+        _configUnavailable = config.Outcome is ApiConfigOutcome.Unavailable
+            || (config.Document is not null && !BuilderConfigMapper.Hydrate(Wizard, config.Document, RegisteredSourceType));
+
+        if (_configUnavailable || config.Document is null)
             return;
 
         Wizard.IsEditing = true;

src/UI/NeoReports.UI/Pages/BuilderConfigure.razor

@@ -40,13 +40,23 @@
             <Button Variant="outline" Size="sm" Icon="circle-check" Loading="_validating" OnClick="ValidateAsync">Validate</Button>
         </Action>
         <ChildContent>
-        @if (Wizard.IsEditing && Wizard.ConnectionStringKept)
+        @* Only where the connection field actually is: a ref-based source has none, so the banner
+           would point at a control that is not on the page. *@
+        @if (Wizard.IsEditing && Wizard.ConnectionStringKept && string.IsNullOrEmpty(Wizard.SourceRef))
         {
             <Banner Variant="info" Icon="lock">
                 <b>The stored connection is kept.</b>
                 <span style="font-size:12px">Its value is not shown — the engine holds back anything that could be the secret itself. Leave the field below blank to keep it, or name an environment variable to replace it.</span>
             </Banner>
         }
+        @if (Wizard.IsEditing && Wizard.SourceChanged && string.IsNullOrEmpty(Wizard.SourceRef)
+             && string.IsNullOrWhiteSpace(Wizard.ConnectionStringVariable))
+        {
+            <Banner Variant="warn" Icon="alert-triangle">
+                <b>Set a connection for the new source.</b>
+                <span style="font-size:12px">You changed the source from what "@Wizard.EditingOriginalName" was reading, so its stored connection was not carried over — restoring it into a different source is not something the engine can assume. Name an environment variable below; saving without one is rejected.</span>
+            </Banner>
+        }
         <div class="col" style="gap:12px">
             <div class="grid-2">
                 <div class="field"><span class="label-sm">Report name</span><span class="input mono"><input class="mono" @bind="Wizard.ReportName" @bind:event="oninput" placeholder="monthly-sales" readonly="@Wizard.IsEditing" style="width:100%" /></span></div>

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

@@ -263,6 +263,7 @@ private static JsonObject BuildSourceProperties(BuilderState state, JsonObject c
         // The connection has its own field on the Configure step, so the carried-over value never
         // survives on its own: it is re-derived below, from that field or from the "keep what is
         // stored" flag. Leaving it in place would make naming a new variable a silent no-op.
+        JsonNode? carriedConnection = Get(carried, ConnectionStringProperty);
         Remove(carried, ConnectionStringProperty);
 
         JsonObject properties = state.UsesAdoSqlShape
@@ -271,8 +272,12 @@ private static JsonObject BuildSourceProperties(BuilderState state, JsonObject c
 
         if (usesRef)
         {
-            // A registered source supplies the connection; sending one here would shadow it.
-            Remove(properties, ConnectionStringProperty);
+            // A registered source supplies the connection, so the wizard offers no field for one —
+            // but a report-local overlay is legitimate configuration (D42: report-local wins), and
+            // deleting a stored one repointed the report at the registry's connection without saying
+            // so. Kept as it arrived; never invented, since there is no field to invent it from.
+            if (carriedConnection is not null && !HasMember(properties, ConnectionStringProperty))
+                SetNode(properties, ConnectionStringProperty, carriedConnection.DeepClone());
         }
         else if (!HasMember(properties, ConnectionStringProperty))
         {

src/UI/NeoReports.UI/Services/BuilderState.cs

@@ -207,6 +207,15 @@ public string ConnectionSummary
     /// </summary>
     public int AdditionalOutputCount { get; set; }
 
+    /// <summary>
+    /// True while editing when the source now selected is not the one the stored document described.
+    /// Everything the previous source carried — its properties and its connection — is deliberately
+    /// dropped in that case, so the Configure step has to say what the user now has to supply.
+    /// </summary>
+    public bool SourceChanged =>
+        !string.IsNullOrEmpty(LoadedSourceIdentity)
+        && !string.Equals(LoadedSourceIdentity, SourceIdentity, StringComparison.Ordinal);
+
     /// <summary>The source currently selected, in the same shape as <see cref="LoadedSourceIdentity"/>.</summary>
     public string SourceIdentity =>
         string.IsNullOrWhiteSpace(SourceRef) ? $"type:{SourceType}" : $"ref:{SourceRef.Trim()}";

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

@@ -56,6 +56,26 @@ public enum ApiCreateOutcome
 /// <summary>Result of <see cref="INeoReportsApiClient.TryCreateReportAsync"/>.</summary>
 public sealed record ApiCreateResult(ApiCreateOutcome Outcome, string? Name, string? Error);
 
+/// <summary>Outcome of a <c>GET /api/reports/{name}/config</c> call.</summary>
+public enum ApiConfigOutcome
+{
+    /// <summary>200 — the stored configuration document was returned.</summary>
+    Ok,
+
+    /// <summary>404 — the report has no stored document: it is code-registered, or gone.</summary>
+    NotFound,
+
+    /// <summary>The engine wasn't reachable, or failed to read the document.</summary>
+    Unavailable,
+}
+
+/// <summary>
+/// Result of <see cref="INeoReportsApiClient.TryGetReportConfigAsync"/>. "Not editable" and "could
+/// not load" are separate outcomes on purpose — collapsing them turned a transient failure into a
+/// silently blank create wizard.
+/// </summary>
+public sealed record ApiConfigResult(ApiConfigOutcome Outcome, string? Document);
+
 /// <summary>A single output column, as returned by <c>GET /api/reports/{name}</c>.</summary>
 public sealed record ApiReportColumn(string Name, string Type, string? DisplayName, string? Format, bool Nullable);
 
@@ -292,12 +312,11 @@ public interface INeoReportsApiClient
 
     /// <summary>
     /// The report's stored configuration document, with credential-bearing values redacted
-    /// (<c>GET /api/reports/{name}/config</c>, ADR D86), or <c>null</c> when the report has none —
-    /// a code-registered report, or an unreachable engine.
+    /// (<c>GET /api/reports/{name}/config</c>, ADR D86).
     /// </summary>
     /// <param name="name">The report name.</param>
     /// <param name="cancellationToken">Cancellation token.</param>
-    Task<string?> TryGetReportConfigAsync(string name, CancellationToken cancellationToken = default);
+    Task<ApiConfigResult> TryGetReportConfigAsync(string name, CancellationToken cancellationToken = default);
 
     /// <summary>Removes a runtime-registered report. Returns whether the engine accepted the request.</summary>
     Task<bool> TryDeleteReportAsync(string name, CancellationToken cancellationToken = default);
@@ -680,22 +699,29 @@ public async Task<ApiCreateResult> TryReplaceReportAsync(
         }
     }
 
-    public async Task<string?> TryGetReportConfigAsync(string name, CancellationToken cancellationToken = default)
+    public async Task<ApiConfigResult> TryGetReportConfigAsync(string name, CancellationToken cancellationToken = default)
     {
         var apiBase = ApiBase;
         try
         {
             using var response = await http.GetAsync(
                 new Uri(apiBase, $"reports/{Uri.EscapeDataString(name)}/config"), cancellationToken).ConfigureAwait(false);
+
+            // 404 is "this report has no stored document" — a real answer. Anything else is a failure
+            // to get one, and the caller has to be able to tell them apart.
+            if (response.StatusCode == HttpStatusCode.NotFound)
+                return new ApiConfigResult(ApiConfigOutcome.NotFound, null);
             if (!response.IsSuccessStatusCode)
-                return null;
+                return new ApiConfigResult(ApiConfigOutcome.Unavailable, null);
 
-            return await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false);
+            return new ApiConfigResult(
+                ApiConfigOutcome.Ok,
+                await response.Content.ReadAsStringAsync(cancellationToken).ConfigureAwait(false));
         }
         catch (Exception ex) when (IsTransient(ex))
         {
             logger.LogWarning(ex, "GET {ApiBase}reports/{Name}/config failed.", Sanitize(apiBase.ToString()), Sanitize(name));
-            return null;
+            return new ApiConfigResult(ApiConfigOutcome.Unavailable, null);
         }
     }
 

tests/NeoReports.AspNetCore.IntegrationTests/ReportEditEndpointsTests.cs

@@ -273,6 +273,51 @@ public async Task Validate_for_rejects_a_document_that_is_not_the_report_it_targ
         result.GetProperty("error").GetString()!.ShouldContain("targets 'sales'");
     }
 
+    [Fact]
+    public async Task Validate_for_does_not_report_the_reports_own_name_as_taken()
+    {
+        using var host = await StartAsync();
+        HttpClient client = await CreateSalesAsync(host);
+
+        string redacted = await client.GetStringAsync("/api/reports/sales/config");
+
+        JsonElement result = await (await SendJsonAsync(client, HttpMethod.Post, "/api/reports/validate?for=sales", redacted))
+            .Content.ReadFromJsonAsync<JsonElement>(Json);
+
+        // Its own name is not taken by anyone else; reporting it as taken put "name already taken"
+        // under every successful edit validation in the Builder.
+        result.GetProperty("valid").GetBoolean().ShouldBeTrue();
+        result.GetProperty("nameTaken").GetBoolean().ShouldBeFalse();
+    }
+
+    [Fact]
+    public async Task Validate_without_for_still_reports_an_existing_name_as_taken()
+    {
+        using var host = await StartAsync();
+        HttpClient client = await CreateSalesAsync(host);
+
+        JsonElement result = await (await SendJsonAsync(client, HttpMethod.Post, "/api/reports/validate", Original))
+            .Content.ReadFromJsonAsync<JsonElement>(Json);
+
+        result.GetProperty("nameTaken").GetBoolean().ShouldBeTrue();
+    }
+
+    [Fact]
+    public async Task A_corrupt_stored_document_is_a_500_on_put_not_a_400_blamed_on_the_client()
+    {
+        using var host = await StartAsync();
+        HttpClient client = await CreateSalesAsync(host);
+
+        var store = host.Services.GetRequiredService<IReportConfigStore>();
+        await store.SaveAsync("sales", "{ this is not json", CancellationToken.None);
+
+        HttpResponseMessage response = await SendJsonAsync(client, HttpMethod.Put, "/api/reports/sales", Original);
+
+        // GET .../config already answers this exact condition with a 500; PUT reported it as a bad
+        // request, blaming the caller for a document on disk they never sent.
+        response.StatusCode.ShouldBe(HttpStatusCode.InternalServerError);
+    }
+
     [Fact]
     public async Task Put_rejects_a_document_whose_name_does_not_match_the_route()
     {