fix: reject the bare placeholder in a section, +5 edit-flow defects

Two of the six are worth carrying forward.

A ref-based ADO report whose query lives in the registered definition got
an empty overlay: those boxes hydrate blank, and the save wrote
"sql": "" / "key": "" into the report-local overlay, which wins under
D42 — so the next run failed with "requires a non-empty 'sql' property"
from a save that changed only the page size. Blank now means "do not
override" for a ref source, and still means "clear it" for an inline one
where the query is the report's own.

The unaddressed placeholder inside a section pulled the SOURCE's
credential. AddressOf returns null for the bare form and null resolves to
source.properties, so a bare placeholder hand-written into a destination —
the only form the README and CHANGELOG document — handed that destination
the source's connection string. Same wire-crossing the addressed form
exists to prevent, third distinct route into it. Rejected outright: only
Redact issues the bare form, and only for the source.

The rest: a column name containing a comma was split in two by the single
text box and both halves retyped as String, so an untouched list is now
written back as the stored array and the step warns when the box cannot
represent a name; validate?for= skipped the restore when the report was
gone, producing a placeholder complaint instead of "that report no longer
exists"; a 404 from GET .../config still degraded to a silent blank
wizard; and destination card selection was still ordinal after the rest of
the destination matching went case-insensitive.

Each fix verified by reverting it and confirming the new test fails — one
revert had to be redone because the first attempt changed only half the
condition and the test passed against it. Full suite: 1 172 green.

Author: thiagoluga

CHANGELOG.md

@@ -74,6 +74,18 @@ The `NeoReports.Abstractions` contract follows SemVer strictly.
   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.
+- **A `ref`-based ADO report keeps the query that lives in its registered definition.** Those boxes
+  hydrate blank, and the save wrote an empty `sql`/`key` overlay — which wins under D42, so the next
+  run failed from a save that changed only the page size.
+- **An unaddressed placeholder inside an output or destination is rejected**, rather than resolving
+  against `source.properties` and handing that section the source's credential.
+- **A column name containing a comma survives an edit.** The single comma-separated box split it in
+  two and retyped both as `String`; an untouched list is now written back as the stored array, and the
+  step warns when the box cannot represent a name.
+- **`validate?for={name}` says when that report has no stored configuration**, instead of complaining
+  about a placeholder the caller sent correctly.
+- **A `404` from `GET .../config` says so** instead of silently becoming a blank create wizard.
+- **Destination card selection matches case-insensitively**, like the rest of the destination matching.
 
 ### Added
 - **`GET /api/reports/{name}/config`** returns a config-origin report's stored document with

DECISIONS.md

@@ -2007,6 +2007,29 @@ breaking; all five were in what surrounds it.
   nothing said so, and the save failed with a generic compile error. The Configure step now says what
   has to be supplied.
 
+### The sixth review pass
+
+Two of these six are the ones worth carrying forward.
+
+- **A `ref`-based ADO report whose query lives in the registered definition got an empty overlay.**
+  Those boxes hydrate blank, and the save wrote `"sql": ""` / `"key": ""` into the report-local
+  overlay, which under D42 wins — so the next run failed with *requires a non-empty 'sql' property*
+  from a save that changed only the page size. Blank now means "do not override" for a `ref` source
+  and still means "clear it" for an inline one, where the query is the report's own.
+- **The unaddressed placeholder inside a section pulled the SOURCE's credential.** `AddressOf` returns
+  null for the bare form and null resolves to `source.properties`, so a bare placeholder hand-written
+  into a destination — the only form the README and CHANGELOG document — handed that destination the
+  source's connection string. This is the same wire-crossing the addressed form exists to prevent,
+  reached from the other direction, and it is the *third* distinct route into it. Rejected outright:
+  only `Redact` issues the bare form, and only for the source.
+
+The other four: a column name containing a comma was split in two by the single text box and both
+halves retyped as `String` (an untouched list is now written back as the stored array, and the step
+warns when the box cannot represent a name); `validate?for=` skipped the restore when the report was
+gone, producing a placeholder complaint instead of "that report no longer exists"; a `404` from
+`GET .../config` still degraded to a silent blank wizard; and destination card selection was still an
+ordinal comparison after the rest of the destination matching went case-insensitive.
+
 ### 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

@@ -721,11 +721,27 @@ private static async Task<IResult> ValidateReportAsync(
             // placeholder would reach a provider as a literal connection string and fail for a
             // reason that has nothing to do with the configuration under test.
             if (http.Request.Query.TryGetValue("for", out StringValues editing)
-                && editing.ToString() is { Length: > 0 } editingName
-                && DynamicReportName.IsValid(editingName)
-                && http.RequestServices.GetService<IReportConfigStore>() is { } store
-                && await store.TryGetAsync(editingName, cancellationToken).ConfigureAwait(false) is { } stored)
+                && editing.ToString() is { Length: > 0 } editingName)
             {
+                string? stored = DynamicReportName.IsValid(editingName)
+                    && http.RequestServices.GetService<IReportConfigStore>() is { } store
+                        ? await store.TryGetAsync(editingName, cancellationToken).ConfigureAwait(false)
+                        : null;
+
+                // Saying so beats silently skipping the restore: the caller would otherwise get
+                // "still holds the redaction placeholder" about a document they sent correctly, plus a
+                // nameTaken flag, for the single real problem that the report is gone.
+                if (stored is null)
+                {
+                    return Results.Ok(new ValidateReportResponse(
+                        Valid: false,
+                        Error: $"There is no stored configuration for '{editingName}' to validate an edit against. " +
+                               "It may have been deleted, or it is code-registered.",
+                        Name: null,
+                        Columns: null,
+                        NameTaken: false));
+                }
+
                 editingFor = editingName;
                 document = ReportConfigSecrets.Restore(document, stored);
             }

src/NeoReports.Core/Configuration/ReportConfigSecrets.cs

@@ -172,8 +172,8 @@ public static string Restore(string document, string storedDocument)
         // 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, new RestoreContext(stored, bag, claims), []);
+        foreach ((JsonObject bag, string? address) in PropertyBagSlots(root))
+            RestoreMembers(bag, new RestoreContext(stored, bag, address, claims), []);
 
         return root.ToJsonString();
     }
@@ -246,8 +246,10 @@ private static void RedactMembers(JsonObject owner, string sentinel)
 
     /// <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="SlotAddress">The address of the slot <paramref name="Bag"/> is, or null for the source.</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 sealed record RestoreContext(
+        JsonObject Stored, JsonObject Bag, string? SlotAddress, Dictionary<string, JsonObject> Claims);
 
     private static void RestoreMembers(JsonObject owner, RestoreContext context, IReadOnlyList<object> path)
     {
@@ -314,6 +316,18 @@ private static void RestoreElements(JsonArray array, RestoreContext context, IRe
 
     private static void ClaimAddress(RestoreContext context, string? address)
     {
+        // The bare placeholder addresses the source, and only Redact issues it — inside an output or
+        // destination bag it can only have been written by hand or pasted from the docs, and honouring
+        // it would hand that section the SOURCE's credential. That is the wire-crossing the addressed
+        // form exists to prevent, reached from the other direction.
+        if (address is null && context.SlotAddress is not null)
+        {
+            throw new ConfigurationException(
+                $"The unaddressed placeholder '{RedactedValue}' was sent inside '{context.SlotAddress}'. It stands " +
+                $"only for a source property; a section's placeholder names its own slot, as in " +
+                $"'{SentinelFor(context.SlotAddress)}'. Send the real value instead.");
+        }
+
         string claimed = address ?? SourceMember;
         if (context.Claims.TryGetValue(claimed, out JsonObject? owner) && !ReferenceEquals(owner, context.Bag))
         {

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

@@ -31,8 +31,16 @@
     @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>
+            @if (_configOutcome == ApiConfigOutcome.NotFound)
+            {
+                <b>That report has no stored configuration to edit.</b>
+                <span style="font-size:12px">It was deleted, or it is registered in code — either way there is nothing here to change. Nothing was prefilled, so this is a blank new report.</span>
+            }
+            else
+            {
+                <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)
@@ -106,6 +114,9 @@
     /// <summary>Set when an edit was asked for and the report's configuration could not be read.</summary>
     private bool _configUnavailable;
 
+    /// <summary>Why it could not be read, so the message can say which of the two happened.</summary>
+    private ApiConfigOutcome _configOutcome = ApiConfigOutcome.Ok;
+
     /// <summary>
     /// Set by ReportDetail's "Edit" button (<c>?edit=name</c>) — hydrates the wizard from that
     /// report instead of starting blank.
@@ -191,10 +202,14 @@
         // "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));
+        // NotFound counts too: Edit is only offered for a config-origin report, so a 404 here means it
+        // was deleted or is code-registered — either way the user asked to edit something and must not
+        // be handed a blank create form without a word.
+        _configOutcome = config.Outcome;
+        _configUnavailable = config.Outcome is not ApiConfigOutcome.Ok
+            || !BuilderConfigMapper.Hydrate(Wizard, config.Document!, RegisteredSourceType);
 
-        if (_configUnavailable || config.Document is null)
+        if (_configUnavailable)
             return;
 
         Wizard.IsEditing = true;

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

@@ -93,6 +93,13 @@
                 </div>
                 <div class="field"><span class="label-sm">Page size</span><span class="input num"><input class="mono" type="number" min="1" @bind="Wizard.PageSize" @bind:event="oninput" /></span></div>
             }
+            @if (Wizard.IsEditing && !Wizard.ColumnNamesAreSplittable)
+            {
+                <Banner Variant="warn" Icon="alert-triangle">
+                    <b>A column name contains a comma.</b>
+                    <span style="font-size:12px">This box separates columns with commas, so it cannot represent that name. Leave it exactly as it is and the stored columns are kept untouched; editing it would split that name into two columns.</span>
+                </Banner>
+            }
             <div class="field"><span class="label-sm">Output columns</span><span class="input mono"><input class="mono" @bind="Wizard.ColumnNames" @bind:event="oninput" placeholder="Id, Customer, Amount" style="width:100%" /></span></div>
             <div class="cgr"><span class="lbl">Track progress</span><div class="ctl"><Switch @bind-Value="Wizard.TrackProgress" /><span class="hint">counts the source rows once before each run · enables a real completion percentage</span></div></div>
             @if (!Wizard.TrackProgress)

src/UI/NeoReports.UI/Pages/BuilderDestination.razor

@@ -35,7 +35,7 @@
                 </SelectableCard>
                 @foreach (var d in _engineDestinations.Select(ToDestinationOption))
                 {
-                    <SelectableCard Selected="@(Wizard.DestinationType == d.Id)" OnClick="@(() => Wizard.DestinationType = d.Id)">
+                    <SelectableCard Selected="@(string.Equals(Wizard.DestinationType, d.Id, StringComparison.OrdinalIgnoreCase))" OnClick="@(() => Wizard.DestinationType = d.Id)">
                         <CatTile Kind="@d.Kind" Icon="@d.Icon" />
                         <div class="name mono">@d.Id</div>
                         <div class="conn" style="white-space:normal;font-family:var(--font-sans);font-size:12px">@d.Description</div>

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

@@ -98,10 +98,21 @@ public static bool Hydrate(BuilderState state, string document, Func<string, str
 
         if (Get(root, "columns") is JsonArray columns)
         {
-            state.ColumnNames = string.Join(", ", columns
+            string[] names = columns
                 .OfType<JsonObject>()
                 .Select(column => (Get(column, "name") as JsonValue)?.ToString())
-                .Where(columnName => !string.IsNullOrWhiteSpace(columnName)));
+                .Where(columnName => !string.IsNullOrWhiteSpace(columnName))
+                .Select(columnName => columnName!)
+                .ToArray();
+
+            state.ColumnNames = string.Join(", ", names);
+
+            // One comma-separated box cannot represent a name that itself contains a comma, so an
+            // untouched list is written back as the stored array rather than re-derived from the text
+            // — otherwise "Total, net" came back as two columns, both retyped as untouched String,
+            // which is the exact downgrade the patch-don't-regenerate rule exists to prevent.
+            state.LoadedColumnNames = state.ColumnNames;
+            state.ColumnNamesAreSplittable = !names.Any(columnName => columnName.Contains(',', StringComparison.Ordinal));
         }
 
         if (Get(root, "outputs") is JsonArray outputs)
@@ -267,7 +278,7 @@ private static JsonObject BuildSourceProperties(BuilderState state, JsonObject c
         Remove(carried, ConnectionStringProperty);
 
         JsonObject properties = state.UsesAdoSqlShape
-            ? AdoSourceProperties(state, carried)
+            ? AdoSourceProperties(state, carried, usesRef)
             : GenericSourceProperties(state, carried);
 
         if (usesRef)
@@ -294,13 +305,29 @@ private static JsonObject BuildSourceProperties(BuilderState state, JsonObject c
 
     // The SQL family has dedicated editors for its query and key column; every other stored property
     // is carried through untouched, since nothing in the wizard could have changed it.
-    private static JsonObject AdoSourceProperties(BuilderState state, JsonObject carried)
+    private static JsonObject AdoSourceProperties(BuilderState state, JsonObject carried, bool usesRef)
     {
-        Set(carried, "sql", state.SqlQuery);
-        Set(carried, "key", state.KeyColumn);
+        SetOverlay(carried, "sql", state.SqlQuery, usesRef);
+        SetOverlay(carried, "key", state.KeyColumn, usesRef);
         return carried;
     }
 
+    /// <summary>
+    /// Writes one of the SQL family's own properties. For an inline source the query and key column
+    /// belong to the report, so a cleared box clears the property and the engine rejects the config —
+    /// the honest outcome. For a <c>ref</c>-based source they are an optional overlay over the
+    /// registered definition (D42), where blank means "do not override": writing an empty one there
+    /// wins over a perfectly good stored query and breaks the next run, from a save that changed
+    /// nothing — because a report whose query lives in the definition hydrates these boxes empty.
+    /// </summary>
+    private static void SetOverlay(JsonObject properties, string key, string value, bool usesRef)
+    {
+        if (!usesRef || !string.IsNullOrWhiteSpace(value))
+            Set(properties, key, value);
+        else
+            Remove(properties, key);
+    }
+
     // The generic editor shows the whole bag, so its rows are the whole bag: anything the user
     // deleted there is meant to be gone, and `carried` only supplies the original JSON types.
     private static JsonObject GenericSourceProperties(BuilderState state, JsonObject carried)
@@ -337,6 +364,11 @@ private static JsonObject GenericSourceProperties(BuilderState state, JsonObject
 
     private static JsonArray BuildColumns(BuilderState state, JsonArray? original)
     {
+        // Untouched: hand back exactly what was stored. The text box is a lossy view of the array,
+        // and re-deriving from it is only safe once the user has actually said something new.
+        if (original is not null && state.ColumnNamesUnchanged)
+            return original.DeepClone().AsArray();
+
         var columns = new JsonArray();
         foreach (string name in state.ColumnNames.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries))
         {

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

@@ -110,6 +110,23 @@ public string ConnectionSummary
     /// <summary>Comma-separated output column names.</summary>
     public string ColumnNames { get; set; } = "Id";
 
+    /// <summary>
+    /// <see cref="ColumnNames"/> as the loaded document produced it, so an untouched list can be
+    /// written back as the stored array instead of re-derived from a box that cannot represent a
+    /// column name containing a comma.
+    /// </summary>
+    public string LoadedColumnNames { get; set; } = "";
+
+    /// <summary>
+    /// False when a stored column name contains a comma, which the single text box cannot round-trip.
+    /// The Configure step warns rather than letting an edit split one column into two.
+    /// </summary>
+    public bool ColumnNamesAreSplittable { get; set; } = true;
+
+    /// <summary>Whether the column box still holds exactly what the loaded document produced.</summary>
+    public bool ColumnNamesUnchanged =>
+        OriginalDocument is not null && string.Equals(ColumnNames, LoadedColumnNames, StringComparison.Ordinal);
+
     /// <summary>Destination type id (e.g. "local", "s3"); empty means no destination configured.</summary>
     public string DestinationType { get; set; } = "";
 
@@ -236,6 +253,8 @@ public void Reset()
         PageSize = 1000;
         TrackProgress = true;
         ColumnNames = "Id";
+        LoadedColumnNames = "";
+        ColumnNamesAreSplittable = true;
         DestinationType = "";
         DestinationPath = "";
         RetryMaxAttempts = 1;

tests/NeoReports.AspNetCore.IntegrationTests/ReportEditEndpointsTests.cs

@@ -318,6 +318,24 @@ public async Task A_corrupt_stored_document_is_a_500_on_put_not_a_400_blamed_on_
         response.StatusCode.ShouldBe(HttpStatusCode.InternalServerError);
     }
 
+    [Fact]
+    public async Task Validate_for_a_report_with_no_stored_document_says_that_plainly()
+    {
+        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=gone", redacted))
+            .Content.ReadFromJsonAsync<JsonElement>(Json);
+
+        // Skipping the restore silently produced "still holds the redaction placeholder" about a
+        // document the caller sent correctly, for the single real problem that the report is gone.
+        result.GetProperty("valid").GetBoolean().ShouldBeFalse();
+        result.GetProperty("error").GetString()!.ShouldContain("no stored configuration for 'gone'");
+        result.GetProperty("nameTaken").GetBoolean().ShouldBeFalse();
+    }
+
     [Fact]
     public async Task Put_rejects_a_document_whose_name_does_not_match_the_route()
     {