Name the settings the viewer lost, and stop one of them costing the rest (#2456)

[erikdarlingdata]

Fixes #2456.

The gap, and why Lite's answer does not port

The viewer's startup dialog could name the file and the parse position and nothing else. That is not a wording problem: it round-trips a whole object, so when JsonSerializer.Deserialize<T> threw there was no property in existence to report — and every setting in the file reverted, not just the bad one.

Option 1 — per-property reads, the shape #2444 gave Lite — is wrong here, and not for the reason the issue weighed. The issue framed it as "is the settings object stable enough to be worth eighty-odd hand-written reads". The decisive problem is smaller and worse than the typing:

Lite's settings.json is built key by key on both sides — #2441 made all ten writers mutate one JsonObject opened once — so a per-key read is symmetric with a per-key write. These files are a whole-object round trip on both sides. Converting only the READ half to hand-written properties breaks that symmetry, and a property added to ViewerAppSettings afterwards would still be serialized on save and silently never read back. That is a worse defect than the one being fixed, and an invisible one; nothing in the compiler or the tests would notice. So the answer had to keep the round trip.

What JsonException actually carries, measured

The issue said JsonException.Path "names one member at best". Measured against the shipped BCL rather than assumed, across every fault class these files can produce:

file	Path	LineNumber
"AlertCpuThreshold": "ninety"	$.AlertCpuThreshold	2
"McpEnabled": "true"	$.McpEnabled	1
"SmtpServer": 5	$.SmtpServer	1
"AlertExcludedDatabases": 5	$.AlertExcludedDatabases	1
"AlertExcludedDatabases": ["a", 2]	$.AlertExcludedDatabases[1]	1
"AlertCpuThreshold": 99999999999999	$.AlertCpuThreshold	1
trailing comma	$	2
unbalanced brace	$	2
root is an array / a string	$	0
two bad members	$.McpEnabled only	1

So it is stronger than "often carries": Path == "$" is exactly the discriminant between the document is broken and one setting is the wrong shape, every time. And the last row is the limit that matters — one member, never the set.

There is a small finding in that. SettingsFileGuard.Describe had the member name in its hand and was deliberately throwing it away: WithoutPathSuffix cuts Path: … | LineNumber: … off the message because it duplicates the line and position it has already rendered — and Path is the one part of that suffix that does not.

The fix: name it, drop it, run the same deserialize again

ReadObject<T> now names the member the deserializer stopped on, removes it from the document, and retries. Two consequences, and the second is the issue's title:

• a badly-shaped setting costs exactly its own setting — the settings before and after it in the file survive, which the single try could never have delivered because it threw and stopped reading;
• the read reports the whole set, which Path alone cannot, because the retry is what surfaces the second and third.

It is one judge, not two. #2213 spent a review round on a two-pass classify whose second pass judged the file by a different standard from the first. Every attempt here is JsonSerializer.Deserialize<T> with the caller's own options over the caller's own type; the only thing that changes between attempts is that one member is gone.

Review found the one place that property could quietly become false, and it is worth naming rather than quietly fixing: dropping a member re-parses the text, and a bare JsonNode.Parse runs with trailing commas and comments disallowed however lenient the caller is. For a permissive caller the deserialize would reach the bad member and the re-parse would throw on the same text, so the recovery would bail to whole-file Unreadable — silently, because that is the pre-#2456 behaviour rather than a crash. Not hypothetical here: darling.json is JSONC, ViewerSettings reads it with AllowTrailingCommas + ReadCommentHandling.Skip, and seven call sites in the repo set those options. The re-parse now borrows the caller's leniency explicitly, and two tests pin both directions — the lenient file recovers its one member, and the same file under strict options is still a document fault, which is the control that stops the first passing on a reader made permanently lenient.

The member name is taken from the path's first segment, up to the first . or [ — which is what the grammar delimits on. Review raised a narrower identifier match as a diacritic edge case; measuring STJ made it wider and more ordinary: the dot form is used for every name needing no escaping, so $.with-dash and $.dollar$sign arrive that way alongside $.ServerNaïve and $.サーバー, and only $['with space'] is bracket-quoted. A hyphenated key is an ordinary thing to find in a settings file. It failed safe either way — the member lookup finds nothing and the read falls back — but it silently lost per-member recovery for exactly the settings someone had to name by hand. ABracketQuotedPathIsStillRefused is the boundary that must not move with it.

Only a top-level member of a root JSON OBJECT is ever dropped, and that restriction is the load-bearing half rather than a simplification. The viewer's server registry is a root array, and a bad entry's path is $[1] — "drop the element that would not deserialize" there means silently deleting a server the operator added, which is the data loss #2434 exists to prevent wearing a repair's clothes. ReadObject_LeavesAnArrayRootedRegistryAllOrNothing is the control for exactly that mistake, and ViewerServerStore says why in place, because a future widening of the recovery is the obvious next edit.

The state stays Unreadable for a partially recovered file rather than becoming some third thing, so PermitReplace still copies the original aside before the next save replaces it. The dropped member's original text exists nowhere else, and the very next save writes the recovered object over it; relaxing the permit would have destroyed the one setting the user actually got wrong, with no copy — strictly worse than dev, where nothing was recovered and everything was preserved. It also costs no new enum member, so nothing in Lite or the service has to learn a fourth state.

The dialog gets two paragraphs, because there are two facts

The viewer could not read these settings files, so the settings they hold are at
their defaults for this session:

viewer-preferences.json — line 4, position 3: …

These individual settings could not be read and are at their defaults for this
session. Everything else in their file loaded normally:

viewer-settings.json — AlertCpuThreshold (The JSON value could not be converted to
                          System.Int32.), McpEnabled (…)

The files have been left exactly as they are. …

A member's line is missing from that second paragraph on purpose, and finding out why is the one thing probing the shipped reader caught that reading it did not. Each retry runs over the document with the previous member removed, and removing it re-serializes — so from the second member onward the exception's position describes the re-serialized text. Measured on a file whose three bad members sit on lines 2, 3 and 4: the first reported line 2, position 22, correctly, and the other two reported line 1, position 30 and line 1, position 14. A position that confidently names the wrong line is worse than none, in a dialog whose whole job is to send someone to the right place in their file — and the member's NAME is the better locator anyway. The document fault keeps its position, where the parse position is the only locator there is, and one test asserts both halves so a rule applied to both cannot quietly take it away.

One list covering both would have to overstate whichever case it was not describing, and the issue is explicit that an overstated capability is worse than an admitted gap. The second paragraph says the settings are named, not that the list is exhaustive of everything wrong with the file — because a document fault after a recovered member is still a document fault, and lands in the first paragraph.

The contract is enforced at the source, and review found the case that broke it. a non-empty UnreadableMembers always comes with a non-null Value — one direction, because the reverse is false: an ordinary Readable file has a value and no members at all. The recovery could drop one member successfully and then meet a fault it could not attribute — a path like $['weird.name'], which System.Text.Json bracket-quotes for a property name that is not a bare identifier — and every such bail returned a null value while still carrying the member list collected on the way. ViewerSettingsFile.Load substitutes defaults for a null value, so every setting reverts, while MainWindow routes on the list and would have said "everything else in their file loaded normally" about a file where nothing loaded at all. Unreachable today (these types are all bool/int/string/List<string>), which is the argument for pinning it: the first property needing a converter that throws ArgumentException makes it reachable with no warning, and the failure is a confident lie rather than a crash. One Unreadable<T> helper is now the only way to build a whole-file failure and it carries no member list, so the invariant holds for every caller rather than being a rule each caller has to remember.

All three stores gained LastLoadUnreadableMembers, including the registry where it is always empty — that is the #2439 lesson taken literally: a guard listing two of the three stores is the scenario-shaped fix that issue was written to prevent.

Verification

Darling.Tests targets net10.0-windows and cannot run on macOS, so the committed test file was compiled into a net10.0 xUnit-shim harness against the real ViewerSettingsFile.cs, ViewerAppSettings.cs, ViewerPreferences.cs, ViewerServerStore.cs, ViewerServerEntry.cs and ViewerLogger.cs — compiled, not transcribed — plus real project references to PerformanceMonitor.Common and PerformanceMonitor.Notifications.

	dev	this branch
the full committed test file	does not compile — 8 errors	16 passed / 0 failed
the identically-expressible subset	4 passed / 6 failed	10 passed / 0 failed

UnreadableMembers does not exist on dev, so the subset asks the same questions through Value, Problem and reflection, which dev has. The six that fail on dev are: one bad setting costing the others, the problem naming the settings, a bad list element costing its own setting only, the store keeping the rest of the file, the three stores answering the member question, and the dialog.

The four that pass on both sides are the controls, and they are what a wrong fix breaks first: a document fault stays all-or-nothing and still says where the parse stopped; the array-rooted registry is never partially recovered; a healthy file is silent and untouched; and the unreadable original is still copied aside before a save replaces it.

Both review-driven cases were additionally checked the way the house asks — revert the fix, watch it go red, put the fix back, with every other case unmoved either way, so they are tests rather than assertions that happen to hold. APartialRecoveryThatCannotFinish_ReportsNoMembersAtAll, TheRecoveryBorrowsTheCallersLeniency_SoBothOfItsReadsAgree and AHandNamedMemberRecoversLikeAnyOther each go red alone against this branch without their fix.

The two landed suites this change sits under were run against the branch as well, since ReadObject is shared: all 13 of #2439's ViewerSettingsFileGuardTests pass (including Load_ReportsUnreadable_WhenAValueHasTheWrongShape, the one most exposed to the recovery), and all 9 of Lite's SettingsFileGuardTests pass — Lite reaches the guard through Read/RootForWrite, which this PR does not touch.

Whole solution builds, 0 errors. CHANGELOG deliberately untouched — #2395 is an open release-prep PR that owns that file for 3.5.1.

#2453 merged while this was open, and the two types coexist

#2453 landed on dev and brought PerformanceMonitor.Common.SettingsValueProblem — (Key, Problem) — for Lite's per-key reader. This PR adds SettingsMemberProblem — (Member, Problem) — for the viewer's whole-object reader. Different names, different files, no collision; rebased onto 88fe948f and re-verified after it merged: whole solution builds 0 errors, and all 31 cases green — this PR's 12, plus all of #2439's ViewerSettingsFileGuardTests run against the rebased branch because ReadObject is shared.

Whether they should become one type is a real question and deliberately not answered here. Unifying them would make the two readers look interchangeable when the whole finding above is that they are not — one is symmetric with a per-key write, the other with a whole-object one — and the decision is better made now that both can be read side by side on dev.

Deliberately not in scope

The .unreadable-<timestamp> copy is still the only route back to a dropped setting's original text. A recovery that also told the user the value it could not read — rather than only the type it could not read it as — would be a better message, and it is one Element.GetRawText() away in SettingsValue's Lite twin; it is not here because it means widening SettingsMemberProblem and deciding how much of a hand-pasted thousand-character value belongs in a MessageBox. Worth its own issue if the dialog gets read in anger.

🤖 Generated with Claude Code

[claude]

Value can come back null even though dropped/UnreadableMembers is non-empty — and that combination is misreported to the user as "everything else loaded normally."

Walk through what happens if the recovery loop drops one member successfully (dropped now has 1 entry) and then hits an unrecoverable fault on the next attempt:

• Here (NotSupportedException/ArgumentException) or at line 380 (member is null || without is null, e.g. a JsonException whose Path doesn't match a top-level member) — both return new SettingsObjectRead<T>(SettingsFileState.Unreadable, null, ..., dropped).
• Value is null, but UnreadableMembers (dropped) is non-empty.

That's the one combination the type's own doc comment (lines 66-71 above) says shouldn't happen: "Value is non-null for Readable and for a file that was read after DROPPING members... UnreadableMembers is what separates the two." Here UnreadableMembers is non-empty but Value is still null.

Downstream, ViewerSettingsFile.Load does read.Value is null ? read with { Value = new T() } : read; — so every setting reverts to defaults, not just the ones named in dropped. But MainWindow.NoteUnreadableSettingsFile routes any file with members.Count > 0 into _unreadableSettingsValues, producing the dialog text "These individual settings could not be read and are at their defaults for this session. Everything else in their file loaded normally." — which would be false in this case; nothing loaded normally.

Given ViewerAppSettings/ViewerPreferences are currently all bool/int/string/List<string>, only JsonException is realistic today, so this looks unreachable in practice right now. But it's a latent trap: the first property that needs a converter capable of throwing ArgumentException/NotSupportedException on bad data (an enum, DateTime, etc.) — or any document-shape fault that resurfaces after a member has already been dropped — would silently reset the whole file while telling the user only a few named settings were lost.

Worth either (a) not entering the _unreadableSettingsValues branch when Value is null despite dropped being non-empty, or (b) having these fallback branches keep whatever partially-recovered value is available instead of discarding it, so the contract documented above actually holds.

[erikdarlingdata]

Correct, and fixed in 9d42f20 — thank you, this was the sharpest possible place to catch it.

You are right that it is unreachable today (ViewerAppSettings and ViewerPreferences are all bool/int/string/List<string>), and that is the argument for pinning it rather than leaving it: the first property needing a converter that throws ArgumentException — an enum, a DateTime — makes it reachable with no warning, and the failure is a confident lie rather than a crash.

I took (a) rather than (b), at the source rather than at the dialog. (b) is not actually available: at the point of an unattributable bail there is no partially-recovered value to keep, because the attempt that would have produced one is the attempt that just failed. So the honest answer is that the file is not partially recoverable, and the named subset is worth nothing there anyway — it is a subset of "all of them".

One Unreadable<T> helper is now the only way to build a whole-file failure and it carries no member list, so the invariant holds for every caller instead of being a rule each caller has to remember. Fixing it in MainWindow would have left the next reader of SettingsObjectRead believing a doc comment that was false.

It cost nothing to report, because the message a whole-file failure carries is now the FIRST failure’s description, kept for the purpose — the only one whose line and position are against the file the user actually has, since every later attempt runs over a re-serialized document. (That is the same trap a5c4e6f took the position off a member for: three bad members on lines 2, 3 and 4 reported "line 2, position 22", then "line 1, position 30" and "line 1, position 14".)

APartialRecoveryThatCannotFinish_ReportsNoMembersAtAll reproduces your scenario with a local type — [JsonPropertyName("weird.name")], which System.Text.Json reports as $['weird.name'] — since no viewer setting has such a name today. Verified it goes red against this branch without the fix and green with it, with the other eleven cases unmoved either way.

[claude]

Review summary

Reviewed this against the T-SQL/C# style guide, Lite/Darling parity, correctness, security, and performance. This PR is entirely C# (SettingsFileGuard.cs + the three Darling viewer stores + MainWindow.xaml.cs), no T-SQL touched.

Parity: No drift. Lite's settings loader (Lite/App.xaml.cs, Lite/Mcp/McpSettings.cs) uses SettingsFileGuard.Read/RootForWrite — the per-key, merge-based path from #2444/#2441 — not ReadObject<T>/DeserializeWithMemberRecovery, which is Darling-only (whole-object round-trip stores). Nothing in Lite needs a matching change here.

Correctness: The retry-and-drop loop is careful about the cases that matter most:

• Array-rooted files (the server registry) correctly stay all-or-nothing (WithoutMember requires a JsonObject root), with a control test pinning it — avoids the Darling's viewer silently replaces a corrupt settings file with defaults, on a click #2434 class of silent data loss.
• Line/position is deliberately dropped for member-level problems since it describes the re-serialized text after removal, not the original file — verified against the documented measurements.
• Good separation between "file unreadable" and "these named settings unreadable" messaging in the dialog, log, and save-quarantine paths.
• Test coverage is thorough, including the coarse-grained "drop the whole list" behavior for a bad element inside AlertExcludedDatabases and the healthy-file no-op control.

Left one inline comment on a genuine (if currently unreachable) contract gap: the recovery loop can return Value = null while UnreadableMembers is still non-empty when it can't complete a retry mid-loop (a NotSupportedException/ArgumentException, or a document-shape fault surfacing after a member was already dropped). That combination isn't supposed to happen per the type's own doc comment, and if it ever fires, the dialog would tell the user "everything else loaded normally" while actually every setting reset to default. Not exploitable today since ViewerAppSettings/ViewerPreferences are all flat bool/int/string/List<string>, but worth tightening before a property type that can throw those exceptions gets added.

Security: No injection surface — this only touches the user's own local %APPDATA% JSON files, no network/process/secrets handling changed.

Performance: Negligible. Settings files are small (~80 keys); worst case is a bounded number of re-parses (capped at MaxDroppedMembers = 500), and it only runs on load/save of local config, not a hot path.

Note: I wasn't able to run dotnet build/the test suite in this sandbox (blocked), so I reviewed statically — worth confirming CI is green before merge.

[claude]

Minor doc-precision nit: the stated invariant is not actually "if and only if."

UnreadableMembers non-empty ⇒ Value non-null does hold (enforced by Unreadable<T> always passing null for members). But the reverse doesn't: a fully Readable file (no members dropped) has Value non-null and UnreadableMembers null — so "Value non-null" does not imply "UnreadableMembers non-empty."

Given this file's own culture of comments that are measured rather than assumed, worth tightening to the one direction that's actually guaranteed, e.g.: "UnreadableMembers non-empty implies Value non-null (the reverse does not hold — a fully Readable file has a non-null Value and no members)." Otherwise a future Debug.Assert written against the literal "iff" wording would fire on the ordinary success path.

[erikdarlingdata]

Right, and worth the edit rather than worth waving through. The forward direction holds and is enforced by Unreadable<T>; the reverse does not, because an ordinary Readable file has a non-null Value and no members at all.

The reason it matters is the one you gave: a Debug.Assert written against the literal "iff" would fire on the happy path — every successful load in the viewer — and whoever wrote it would be right to trust the comment. A comment that overstates is the same defect as a message that overstates, one level down, which is the whole subject of this PR.

Fixed: it now states the one guaranteed direction, with the reason the reverse is false written in place so nobody tightens it back, and the test's summary says the same thing and names ReadObject_SaysNothingAboutAHealthyFile as the case covering the other direction.

[claude]

Review summary

This is a pure C# change (PerformanceMonitor.Common/Services/SettingsFileGuard.cs + Darling viewer stores/MainWindow.xaml.cs + a new test file) — no T-SQL is touched, so the collector style rules (OPTION(RECOMPILE), alias conventions, etc.) don't apply here, and there are no missing-index-DMV concerns to flag.

Correctness. Traced DeserializeWithMemberRecovery end to end:

• The regex-based TopLevelMember extraction (^\$\.([A-Za-z_][A-Za-z0-9_]*)) correctly matches only bare-identifier dot-paths, the same class of paths JsonException.Path emits in dot notation — non-identifier/array-rooted/document-level paths correctly fall through to the existing all-or-nothing behavior rather than mis-extracting a truncated name.
• WithoutMember's use of default JsonNode.Parse (no AllowTrailingCommas/comment handling) is consistent with the s_jsonOptions actually used by all three viewer stores (WriteIndented only), so there's no latent mismatch between what the primary deserializer accepts and what the retry re-parser accepts.
• The Value non-null / UnreadableMembers invariant that SettingsObjectRead<T> depends on (and that the viewer's dialog routing trusts) is correctly enforced at the one choke point (Unreadable<T> always nulls out members) — confirmed the specific regression case called out in the PR body (partial recovery followed by an unattributable fault) can't produce a null Value with a non-empty member list. Left one inline nit on the XML doc's "if and only if" phrasing of that invariant, which is one-directional in practice (a fully Readable file has Value non-null with no members) — not a functional bug, just imprecise wording in a codebase that otherwise measures its comments carefully.
• The array-rooted registry control (ViewerServerStore always reporting empty UnreadableMembers) is correctly wired — confirmed there's no path from DeserializeWithMemberRecovery that would drop an array element, since WithoutMember requires a JsonObject root.

Lite/Darling parity. No drift. SettingsFileGuard.ReadObject<T>/DeserializeWithMemberRecovery is new and Darling-only usage — grepped Lite/ and confirmed Lite's readers (App.xaml.cs, Mcp/McpSettings.cs) only call SettingsFileGuard.Read (the pre-existing document-level JSON-object reader from #2434), not ReadObject<T>. The PR body's own reasoning for why Lite's per-key shape (#2444) doesn't port here checks out against the code: Lite's settings.json really is built/read key-by-key on both sides, while the viewer round-trips whole objects through JsonSerializer. This is a justified, documented asymmetry rather than an accidental one.

Security. No new file/network/process surface — same fixed %APPDATA% paths as before, no user-controlled paths, nothing that touches SQL.

Performance. Bounded retry loop (MaxDroppedMembers = 500) over small local JSON config files, on a cold-start/error path only — no hot-path or collector-query concern.

No functional bugs found. One minor doc-wording nit posted inline.

[claude]

WithoutMember re-parses with JsonNode.Parse(text) using default JsonDocumentOptions, but the member-drop retry in DeserializeWithMemberRecovery deserializes the same text with the caller's own JsonSerializerOptions options (JsonSerializer.Deserialize<T>(current, options)).

Those two option sets aren't always equivalent — JsonSerializerOptions.AllowTrailingCommas / ReadCommentHandling have no effect on this JsonNode.Parse call, which always runs with trailing-commas-disallowed / comments-disallowed. If a future caller passes ReadObject<T> a comment- or trailing-comma-tolerant options (this codebase already does that elsewhere, e.g. ViewerSettings.cs's s_jsonOptions and the Darling service's config readers all set AllowTrailingCommas = true + ReadCommentHandling = Skip), then for any file that relies on that leniency, the first JsonSerializer.Deserialize<T>(current, options) call would succeed right up to the bad member, but the retry's JsonNode.Parse(current) here would throw on the trailing comma/comment, return null, and DeserializeWithMemberRecovery would bail out to a full-file Unreadable instead of doing the intended per-member recovery.

It's a silent regression only for that combination — since it falls back to the old (pre-#2456) all-or-nothing behavior rather than crashing, and none of today's three viewer stores use permissive JsonSerializerOptions, so it's dormant right now. But ReadObject/DeserializeWithMemberRecovery are written as general-purpose, reusable Common APIs (per the class doc comment), so this is a trap for the next caller. Worth threading options through, e.g.:

var docOptions = new JsonDocumentOptions
{
    AllowTrailingCommas = options?.AllowTrailingCommas ?? false,
    CommentHandling = options?.ReadCommentHandling ?? JsonCommentHandling.Disallow,
    MaxDepth = options?.MaxDepth ?? 0,
};
...
if (JsonNode.Parse(text, documentOptions: docOptions) is not JsonObject root || !root.Remove(member))

[erikdarlingdata]

Correct, and it lands on the load-bearing claim rather than beside it — fixed in 6ac8c8b.

I argued this recovery is "one judge, not two": every attempt being JsonSerializer.Deserialize<T> with the caller's own options, so the two reads cannot disagree about what unreadable means. That was true of the deserializes and not of the read between them, which is the one place the property could quietly become false. Thank you for going and looking there.

Premise checked before acting on it: ViewerSettings.cs:118-123 really does set ReadCommentHandling = Skip + AllowTrailingCommas = true (darling.json is JSONC), and seven call sites in the repo set those options. So the next caller of a general-purpose Common API is as likely to be lenient as strict, and the failure would be silent because it falls back to pre-#2456 behaviour rather than crashing.

Took your suggested mapping including MaxDepth, with the reason written at WithoutMember where the next person is tempted to simplify it back to a bare Parse.

Two tests rather than one. TheRecoveryBorrowsTheCallersLeniency_SoBothOfItsReadsAgree is your scenario — a file with a comment AND a trailing comma AND one bad member, read with lenient options, which must recover just that member. TheSameFileUnderStrictOptionsIsStillADocumentFault is the control: the same file under default options is still all-or-nothing. Without it the first would pass on a reader made permanently lenient, which is a worse defect in the other direction — every strict caller silently accepting JSONC.

Verified the new case goes red against this branch without the fix (Value cannot be null) and green with it, with the other thirteen unmoved either way.

[claude]

Review

This is a C# change to the Darling viewer's settings-file recovery (no T-SQL touched here, so the collector-style conventions in CONTRIBUTING.md don't apply). Walked SettingsFileGuard.DeserializeWithMemberRecovery/WithoutMember, the three viewer store changes, MainWindow's dialog wiring, and the new xUnit test file against the production code they exercise.

Correctness: traced the retry loop (member-name extraction via the anchored $. regex, drop-and-reserialize, the array-root bail, the "recovered-then-hit-an-unattributable-fault" null-value-with-no-members invariant) against the test suite's expectations — the logic and the tests agree, and the design choices (why not per-property reads, why not two readers, why the array registry stays all-or-nothing, why partial recovery keeps State = Unreadable so PermitReplace still quarantines) all hold up. Left one inline finding: WithoutMember's reparse (JsonNode.Parse(text)) always runs with default JsonDocumentOptions, while the sibling JsonSerializer.Deserialize<T>(current, options) call honors the caller's JsonSerializerOptions. For a future caller of ReadObject<T>/DeserializeWithMemberRecovery that passes trailing-comma/comment-tolerant options (this codebase already does that elsewhere, e.g. ViewerSettings.cs), the member-recovery retry would silently fail to reparse and fall back to full-file Unreadable for files that rely on that leniency. It's dormant today since none of the three viewer stores use permissive options, but worth closing given SettingsFileGuard is explicitly written as reusable Common infrastructure.

Lite/Darling parity: SettingsObjectRead<T>/ReadObject/PermitReplace in PerformanceMonitor.Common/Services/SettingsFileGuard.cs are shared, but confirmed Lite doesn't call any of them (Lite reaches the guard only through Read/RootForWrite, which this PR doesn't touch) — so no drift introduced, consistent with the PR description's explanation that Lite's per-key reader (#2444, SettingsValueProblem) is a deliberately different, non-unified answer to the same complaint for a different (per-key-written) file shape.

Security: no new file/process/network surface; JSON handling stays within System.Text.Json; no injection vectors.

Performance: the retry loop is bounded (MaxDroppedMembers = 500) and only runs at settings-file load time on small local JSON files, not a hot path.

[claude]

Minor/low-severity edge case: s_topLevelMember only matches ASCII identifier characters ([A-Za-z_][A-Za-z0-9_]*). If a top-level member name contains a non-ASCII character (e.g. a diacritic), the regex will still match the ^\$\. prefix but capture only the ASCII-matching prefix of the name (e.g. ServerNaïve → ServerNa), then WithoutMember will call root.Remove("ServerNa"), which returns false since no such key exists, and the read correctly falls back to whole-document Unreadable rather than doing anything wrong. So this fails safe today — no data-loss risk — but it's worth a comment (or widening the char class to \p{L}) since none of the three viewer settings types currently has a non-ASCII property name, and a future one added via [JsonPropertyName(...)] would silently lose per-member recovery for that one setting without any test catching it.

[erikdarlingdata]

Right that it fails safe, and right that it was worth raising — fixed in f6c180d. Measuring System.Text.Json rather than reasoning about it made the case both wider and much more ordinary than the diacritic:

$.ServerNaïve      dot form
$.サーバー           dot form
$.with-dash        dot form
$.dollar$sign      dot form
$['with space']    bracket form

STJ writes the DOT form for every name that needs no escaping and bracket-quotes only where it must. So the identifier class was capturing with out of with-dash — and a hyphenated key is an ordinary thing to find in a settings file, not an exotic one. Your own example turned out to be the least likely of the four to show up.

Took the segment to the first . or [ rather than widening to \p{L}, since that is what the path grammar actually delimits on and it covers $ and - too, which \p{L} would not.

The boundary that must not move with it has its own test — ABracketQuotedPathIsStillRefused — because the reader has no business editing a document it cannot name a member in unambiguously, and widening the capture is exactly the change that would erode that.

Pinned with the hyphen and the dollar sign rather than the diacritic, those being the two a real settings file is likely to contain. Verified the new case goes red against this branch without the fix and green with it, with the other fifteen unmoved.

[claude]

Review summary

Scope: this PR touches only the Darling viewer and PerformanceMonitor.Common/Services/SettingsFileGuard.cs. No T-SQL is touched, so the collector style rules don't apply here.

Lite/Darling parity: No drift. Confirmed by grepping call sites — Lite only calls SettingsFileGuard.Read/RootForWrite (its own #2444 per-key reader), never ReadObject/PermitReplace, so the new member-recovery path in DeserializeWithMemberRecovery cannot change Lite's behavior. This matches the PR description's claim and is worth taking at face value.

Correctness: Traced the retry loop in DeserializeWithMemberRecovery against several edge cases not explicitly covered by the new test file:

• Array-rooted registry: TopLevelMember's regex requires the $. prefix, so a bad element's path ($[1], $[1].ServerName) never matches — the registry correctly stays all-or-nothing without needing a special case.
• Bracket-quoted paths ($['weird.name']) also don't match ^\$\., correctly falling back to whole-document Unreadable rather than misattributing.
• Duplicate-key JSON, JsonNode round-tripping of numeric literals through WithoutMember's re-serialize, and the interaction between PermitReplace's re-read at save time and a partially-recovered Load all check out — the state staying Unreadable for a partial recovery is what keeps PermitReplace quarantining correctly, per Save_StillCopiesAPartiallyRecoveredFileAside.
• Left one inline comment on a minor, fails-safe edge case in the member-name regex (ASCII-only identifier matching) — not a bug today, just a latent gap worth a comment given none of the three settings types currently has a non-ASCII property name.

Security: No new attacker-controlled surface — these are local per-user JSON config files the app itself writes; no secrets are stored in the recovered object types (SMTP password / webhook URLs live in Credential Manager, confirmed by grep). No format-string or injection concerns in the new Summarize/ToString helpers.

Style: New public members (SettingsMemberProblem, LastLoadUnreadableMembers ×3) all have XML doc comments; new test file has the copyright header; naming conventions match the surrounding code.

Caveat: I could not run an actual dotnet build/test in this sandbox (Linux, no Windows/WPF SDK, and build commands were blocked by the permission layer), so I relied on static reading rather than compiling. The PR description's own build/test verification (shim harness against the real files, 0 errors) is consistent with what the code reads as.

Nothing blocking found.

[claude]

Reviewed. This is a C# change to Darling's viewer settings loading (SettingsFileGuard.ReadObject<T> member-recovery), not T-SQL, so the T-SQL style rules don't apply here.

Correctness — traced the retry loop in DeserializeWithMemberRecovery/WithoutMember/TopLevelMember carefully:

• The SettingsObjectRead<T> invariant ("non-empty UnreadableMembers ⇒ non-null Value") is correctly enforced at the single choke point (Unreadable<T> always passes null for members) — matches the review-found bug the PR description calls out.
• Array-rooted documents (the server registry) are correctly excluded from recovery: TopLevelMember requires a literal $. prefix, so an array path like $[1] never matches, and WithoutMember double-checks the root is a JsonObject anyway. Good defense in depth against the Darling's viewer silently replaces a corrupt settings file with defaults, on a click #2434 data-loss class.
• The leniency-borrowing fix in WithoutMember (passing the caller's AllowTrailingCommas/ReadCommentHandling/MaxDepth into the re-parse's JsonDocumentOptions) is correct — verified none of the three Darling stores (ViewerAppSettings, ViewerPreferences, ViewerServerStore) currently pass lenient options, so this is genuinely dormant-but-correct for a future caller, as the comment claims.
• Cumulative member removal across retries is correct (each pass operates on the previous pass's already-shrunk JSON), and the position/line-number omission for member-level problems (vs. keeping it for document-level faults) is right, since positions after the first drop describe a re-serialized document rather than the file on disk.
• TopLevelMember's regex (^\$\.([^.\[]+)) capturing up to first ./[ rather than an identifier class is the right call for hyphenated/symbol property names STJ renders in dot form — confirmed against STJ's actual escaping rules.

Lite/Darling parity — no drift here. SettingsFileGuard.ReadObject<T>/member-recovery is Darling-only; Lite doesn't call ReadObject/PermitReplace at all (only Read/RootForWrite, confirmed via grep), because Lite's settings.json is a key-by-key JsonObject round trip on both read and write (#2441/#2444/#2453), while Darling's viewer stores round-trip a whole typed object. The PR body's reasoning for why porting Lite's per-key reader here would silently break the write/read symmetry checks out. SettingsValueProblem (Lite) vs. SettingsMemberProblem (Darling) coexisting as separate types for this reason is a deliberate, documented decision, not an oversight.

Security/perf — no concerns. Purely local JSON file parsing (no network/DB/process boundaries), the retry loop is capped (MaxDroppedMembers = 500) so it can't loop unboundedly on a pathological file, and the regex has no backtracking risk.

One low-severity note, not a blocker: DeserializeWithMemberRecovery drops the entire top-level member on any failure under it (e.g. $.Parent.Child drops all of Parent, not just Child). This is called out and tested as intentional ("coarse on purpose") for list-valued settings, and none of the three current settings types (ViewerAppSettings, ViewerPreferences, ViewerServerEntry) have nested object properties, so it's not reachable today — just worth remembering if a future setting introduces a nested object, since a single bad leaf would then cost sibling values that were otherwise fine.

Nothing else stood out — the test suite already covers the edge cases I went looking for (multi-member drops, document-vs-member fault boundary, hand-named/bracket-quoted members, the partial-recovery-then-unattributable-fault combination, the lenient-options seam).