Make one badly-shaped setting cost one setting (#2444)

[erikdarlingdata]

Closes #2444.

The defect

App.LoadAlertSettings wrapped all eighty-seven reads in one try. #2425 took the parse out of it; the value half was untouched. A key holding a string where an int belongs threw on its own Get* call and abandoned every read after it, so which settings survived depended on where the bad key sat in the file — and that ordering is an implementation detail of the method's line order.

Measured against dev's own source rather than described: alert_cpu_threshold is read with v.GetInt32() 12,389 characters before analysis_timeout_seconds, inside a single 13,902-character try with no inner try. GetInt32() on a JSON string throws. So {"alert_cpu_threshold": "ninety"} costs analysis_timeout_seconds and every other key below it.

The two judgement calls the issue named

A read helper that carries the key name — yes. Eighty-seven try blocks would work and would be unreadable. SettingsReader checks ValueKind before it calls a getter and records the key it could not read. That is the call McpSettings.Load already makes, for the reason it gives on the line: a catch around GetBoolean is what turned a quoted "true" into a silently disabled endpoint, and a hand-edited file is exactly where a quoted boolean comes from.

Report the whole set — yes, and it is the same fix. Someone who edited one line has one mistake; someone who pasted a block out of settings.sample.json has several, and stopping at the first means they fix, restart, and discover the next one, several times over. The old code could not have done this even in principle — it threw on the first bad value and never reached the rest — so "which key" and "all of them" are not two decisions.

The extractor trap

The key literals stay in the shape the extractor already expects, and the extractor is unchanged. SettingsReader's method is called TryGetProperty, so a call site still reads read.TryGetProperty("alert_cpu_threshold", out v) and SettingsSampleTests's regex matches it exactly as before. The name is load-bearing, not a stylistic echo, and it is documented as such at the definition, at the call site, and in the guard.

Keeping the shape was the lower-risk half of the choice the issue offered — but it creates a new dependency held together by nothing the compiler checks, so the guard gets the self-test the issue asked for anyway:

• Both call shapes are seen — the pre-One badly-shaped value still costs every Lite setting after it, and the message can't say which key #2444 root.TryGetProperty(…) and the new read.TryGetProperty(…) — over a source the test writes.
• It still catches an undocumented key. The synthetic sample is missing one key on purpose and the real UndocumentedKeys comparison names it. A guard that extracts keys but no longer compares them is the same vacuous green as one that extracts nothing, and the only thing standing between that and a green build was a floor of >= 50.
• The trap itself is measured. The shape Say when settings.json cannot be read, and keep it when it cannot (#2425) #2428 tried — a read helper taking the key as an ordinary argument — is run through the same extraction and yields nothing. That is what makes the name load-bearing rather than a claim that it is.
• The name is asserted directly, so a rename fails there, with the reason, instead of as an unexplained collapse of the key count elsewhere.
• The scoping contract gets its own case: each read belongs to the most recent *.json literal above it, which is how servers.json and collection_schedule.json's loaders stay out of this set without an exemption — and how a loader moved above the settings.json literal would silently leave it.

To do any of that, the extraction and the comparison are split out to run over text. Both are the shipped ones the real tests call, not copies.

The literal is also kept out of the new comments. LoadDefaultTimeRange already warns that the extractor cannot tell a comment from code and would read an example as a real key; the first draft of this change put TryGetProperty(" in two prose lines and would have injected two junk keys into the extracted set.

Two things that fall out

A latent overflow, now unreachable. Two of the inline clamp forms were (int)Math.Max(0, v.GetInt64()) — floor, then narrow. A hand-typed value beyond int range wrapped, so a bigger threshold became a negative one. Int(fallback, min, max) clamps before it narrows.

A second throw site the single try hid. alert_excluded_databases called elem.GetString() on every element; a number in that array threw, and cost every setting after it. It now filters by element kind, which the fleet-group list beside it already did.

The message becomes visible, and specific

The value fault logged and stopped there, so a user whose thresholds had silently reverted had no signal unless they went looking. #2425 had already decided, for the document half, that someone looking at an app which has forgotten its configuration should not have to find a log to learn why; the same argument applies to a setting that reverted. What is new is that the message can name the keys and say everything else loaded — which is what makes it worth a dialog rather than noise, and is the difference from the message that used to send someone to proofread an eighty-seven-key file.

It reuses the one startup modal #2425 added rather than opening a second. The two cannot both happen on one run: a document that will not parse never reaches a value read.

The catch stays with no known cause left in it, so an unforeseen throw cannot take startup down — and its comment now says what it still costs when it fires, because the reads are ordered and anything after a throw is still lost.

Where the reader lives, and the load/save mental model

PerformanceMonitor.Common, beside SettingsFileGuard, not in Lite. #2441 rewrote SaveAlertSettings so all ten writers mutate one document opened once through App.SettingsRootForWrite() and written once; the load side now has the matching shape — one document opened once through SettingsFileGuard.Read, one reader over it, one report at the end. Both sides treat a per-key failure as a value to carry rather than an exception to throw, and neither can now lose the rest of the file to one key.

It is in Common rather than Lite because that is the seam the Viewer's dialog needs for the gap #2444 names one level up: it round-trips a whole object, so its deserialize fails before any property exists and it cannot say which settings were lost either. Deferred, not folded in — the Viewer's loader is a different mechanism (whole-object deserialize, not per-property reads) and converting it is its own change with its own tests.

Verification

Lite.Tests targets net10.0-windows and cannot run on macOS, so everything runnable was compiled into a net10.0 xUnit-shim harness against the real PerformanceMonitor.Common build, with the real App.xaml.cs and settings.sample.json as fixtures.

• 13 passed / 0 failed on this branch.
• That includes all four pre-existing SettingsSampleTests, which is the result that matters most here — they are the guard this refactor was most likely to break, and they are green with two-way symmetry over all 89 documented keys and both exemption sets still empty.
• Against dev the same file runs 6 passed / 0 failed with the one test that needs the new type removed. The extraction self-tests are guards, not regression tests, and pass on both sides by design — that is what they are for.
• Everything in SettingsValueReadTests that exercises SettingsReader cannot compile against dev, which is the stronger statement.

The two loader-level cases need WPF and can only run on CI, so their premise was measured rather than assumed — the character offsets and the single try above, plus GetInt32()'s throw, both taken from the shipped dev source and the shipped BCL in the harness. Those are exactly the two facts those cases assert the absence of. Stating the scope precisely: the reader and the guard ran here; the two end-to-end loader cases did not, and CI is the arbiter for them.

No CHANGELOG entry, matching #2425, #2428, #2432 and #2442: [3.5.0] is a shipped, dated section and there is no [Unreleased] on dev.

🤖 Generated with Claude Code

[claude]

Reviewed the diff (Lite/App.xaml.cs, the new PerformanceMonitor.Common/Services/SettingsValueReader.cs, and the two Lite.Tests files). No T-SQL in this PR, so the CONTRIBUTING.md T-SQL section doesn't apply; checked against the C# style section instead.

Correctness — verified, no bugs found. I independently re-derived every one of the ~50 clamped/unclamped call sites in LoadAlertSettings/LoadDefaultTimeRange against the pre-PR code line by line (ranges, fallback values, and int/long field types) and they all match. The two previously-latent (int)Math.Max(0, v.GetInt64()) floor-then-narrow overflow bugs (alert_low_disk_threshold_gb, alert_disk_critical_free_gb, alert_file_growth_rise_mb, alert_pvs_floor_gb) are correctly fixed by clamping in Int(fallback, min, max) before narrowing.

Regex self-test claims verified. Confirmed by hand that none of the new/rewritten XML-doc comments in App.xaml.cs or Lite.Tests/SettingsSampleTests.cs accidentally contain a literal TryGetProperty(" sequence that would inject a phantom key into SettingsSampleTests' extraction (the PR explicitly calls this trap out, and it avoided it). Also confirmed the new "settings.json" literal added in ReportBadSettingValuesToUser's dialog text sits before the two real loader calls in file order, so it doesn't perturb the extractor's openFile state tracking.

Lite/Darling parity. Darling/PerformanceMonitor.Darling.Viewer/ViewerAppSettings.cs uses a whole-object deserialize + Normalize() clamp, a genuinely different mechanism from Lite's per-property TryGetProperty reads — this isn't parity drift introduced by this PR, and the description's "deferred, not folded in" framing checks out against the actual code.

Minor, non-blocking nit: ReportBadSettingValues's log line ("...Every OTHER setting in the file loaded normally") is shared between LoadDefaultTimeRange and LoadAlertSettings, but LoadDefaultTimeRange runs first (App.xaml.cs calls it before LoadAlertSettings). If default_time_range_hours itself is malformed, that log line fires before alert settings have even been read, so "every other setting loaded normally" is momentarily inaccurate in the log (the aggregated startup dialog the user actually sees is correct since it's shown after both loaders run). Log-only, cosmetic, not worth blocking on.

Test coverage (SettingsValueReadTests.cs) looks appropriately targeted — the position-sensitive fixtures (bad key with good keys on both sides) are exactly what's needed to distinguish this fix from the old single-try behavior, and I confirmed the asserted Problem message substrings match what SettingsValue.Reject actually produces.

[claude]

Minor edge case: Int(fallback, min, max) and Long(fallback, min, max) read via TryGetInt64 specifically so an out-of-range number clamps to the bound instead of being reported as a shape problem (per the doc comment above Int and the AnOutOfRangeNumberClampsToTheBound_RatherThanWrappingNegative test). That holds only up to long range. A value beyond long.MaxValue/below long.MinValue (e.g. "alert_low_disk_threshold_gb": 99999999999999999999) makes TryGetInt64 return false, so it falls into the Reject(fallback, "a whole number") branch instead of clamping to max — the startup dialog would tell the user this "holds a JSON number where a whole number belongs," which is a confusing message for a value that plainly is a whole number, just an extreme one. Not a practical concern for most fields, but worth either clamping via decimal/string-magnitude sniffing or at least acknowledging the boundary in the comment so it doesn't read as an absolute guarantee.

[claude]

Reviewed the diff (App.xaml.cs, the new SettingsValueReader/SettingsReader in PerformanceMonitor.Common, and the Lite.Tests additions).

Correctness. The per-key rejection model is sound: SettingsReader.TryGetProperty + SettingsValue.{Bool,Int,Long,Double,Text,TextOrNull,IsArray} check ValueKind before converting and record a SettingsValueProblem instead of throwing, so one bad key genuinely no longer costs every read after it. Verified the call ordering (LoadDefaultTimeRange then LoadAlertSettings, both before ReportUnreadableSettingsToUser) means s_badSettingValues is fully populated before the one dialog fires, and that the document-unreadable path and the per-value path are mutually exclusive as claimed. The int64-then-narrow clamp genuinely fixes the old (int)Math.Max(0, v.GetInt64()) wraparound bug. Left one inline note: the clamped Int/Long overloads read via TryGetInt64, so a value beyond long range falls through to "wrong shape" instead of clamping to max — a narrow edge case, not a practical issue.

Test coverage. SettingsValueReadTests and the extended SettingsSampleTests are unusually rigorous — they pin the reader's policy (every bad key reported, absent-key-is-not-a-problem, clamp-not-throw, shape-vs-unrecognized-value distinction) and separately prove the key-extraction regex still matches the new SettingsReader.TryGetProperty call shape (guarding against the exact drift PR #2428 hit).

Lite/Darling parity. This is Lite-only, which is correct: Darling's config loading (DarlingConfig.cs) doesn't use the per-key TryGetProperty pattern this fixes, so there's no counterpart drifting out of sync. The PR body's note that the Viewer's whole-object-deserialize loader has a related but distinct gap, deferred to a follow-up, matches what's actually in the Viewer code.

Security. No new file/network/process surface; webhook/credential handling paths are unchanged other than routing through the new shape-checked reads, and secrets still load unconditionally before any settings.json parsing as before.

No blocking issues found.

[erikdarlingdata]

Addressed the inline finding on SettingsValueReader.cs:151 — it was right, and it was worse than an odd message: the clamped readers promised "an out-of-range number clamps to the bound" and that promise silently stopped at Int64, so 99999999999999999999 fell into the wrong-shape arm and the startup dialog told a user their whole number "holds a JSON number where a whole number belongs".

Fixed by clamping by SIGN rather than by value. A number beyond Int64 in either direction cannot be inside any range a caller can express, so the only question left is which end — read off the raw token rather than through a double, because long.MaxValue does not survive that round trip and a bound near it would come back wrong in the one case this exists for. The promise is now absolute rather than "up to Int64".

The unclamped Int(fallback) is the other side of that boundary and still reports, because with no range there is nothing to put the number into — but it now reports it as out of range and names the value, truncated at forty characters since a hand-edited file is where a thousand-digit number comes from and this string reaches a dialog.

Two tests pin both sides (ANumberBeyondInt64_StillClampsToABound, AnUnclampedReadReportsAnOutOfRangeNumber_AsOutOfRangeRatherThanWrongShape). The earlier nit about the shared log line claiming "every other setting in the file loaded normally" before LoadAlertSettings had run is also fixed — it now claims only what its caller can see, and the whole-file statement stays where it was already correct, in the dialog shown after both loaders.

Both branches have been rebased onto dev (60c696f) and re-verified; the harness runs 15 passed / 0 failed.

[claude]

Fractional numbers silently clamp to the bound with no Problems entry — for every clamped int/long setting.

Element.TryGetInt64 returns false for any JSON number that isn't a pure integer token — not just ones that overflow Int64. A hand-typed 30.0 or 5.5 is a syntactically valid JSON number (ValueKind == Number), but TryGetInt64 fails on it the same way it fails on 99999999999999999999, and the fallthrough treats every TryGetInt64 failure as "magnitude out of range":

: Element.TryGetInt64(out var number) ? (int)Math.Clamp(number, min, max)
: IsNegative() ? min : max;

So e.g. { "analysis_timeout_seconds": 30.0 } (meant to be 30) silently becomes 600 (the max bound), { "alert_cooldown_minutes": 5.5 } silently becomes 120, etc. — without adding anything to Problems, so it never reaches ReportBadSettingValues/the startup dialog/the log. This affects every setting that goes through the clamped Int(fallback, min, max) or Long(fallback, min, max) overloads (connection_refire_minutes, analysis_timeout_seconds, alert_cooldown_minutes, ag_redo_queue_alert_kb, …).

This directly undercuts the point of #2444: a bad value is supposed to be named, not silently substituted with a value the user never asked for and never sees flagged. The unclamped Int(fallback) overload above doesn't have this problem — it goes through RejectAsOutOfRange, which at least records a Problems entry (even if "out of range" is a slightly odd description for "not a whole number").

Suggest checking TryGetInt64's failure reason (e.g. try TryGetInt32-style shape validation first — does the raw text look like an integer? — or just fall back through Reject(fallback, "a whole number") when the token isn't purely digits/sign) before assuming the only way TryGetInt64 fails is overflow. Worth a test case alongside ANumberBeyondInt64_StillClampsToABound for something like { "n": 5.5 } asserting Problems is non-empty.

[claude]

Review

Scope: C#/WPF only (Lite/App.xaml.cs, PerformanceMonitor.Common/Services/SettingsValueReader.cs, Lite.Tests/*) — no T-SQL touched, so the T-SQL style section doesn't apply here.

Lite/Darling parity: No drift. The reader intentionally lives in PerformanceMonitor.Common rather than Lite, but Darling's Viewer settings load (ViewerAppSettings.cs/ViewerSettingsFile.cs) is a whole-object JsonSerializer.Deserialize, not the per-key TryGetProperty pattern this PR fixes — confirmed by reading those files. The PR body explains this is deferred as a separate change rather than an oversight, which checks out.

Bug found: left an inline comment on SettingsValueReader.cs (the clamped Int/Long overloads). TryGetInt64 returns false for any non-integer JSON number, not just ones that overflow Int64 — so a hand-typed fractional value like 30.0 or 5.5 for a clamped setting (analysis_timeout_seconds, alert_cooldown_minutes, ag_redo_queue_alert_kb, etc.) silently jumps to the min/max bound based on sign, with no Problems entry, so it never surfaces in the log or the startup dialog. That's a real gap against the PR's own stated goal (every bad value gets named), since it's easy to trigger by hand-editing settings.json with a .0 typo.

Everything else — the SettingsReader/SettingsValue design, the per-key reporting, the SettingsSampleTests extraction self-tests, the array-element filtering fix, the Int64-narrowing-overflow fix — looks correct and well covered by the new tests. No security concerns (no new file/network/process surface; settings.json is local, already-trusted input) and no missing-index-DMV concerns (not applicable, no T-SQL).

[erikdarlingdata]

Fixed, and thank you — this was a defect I introduced one commit earlier while fixing the previous finding in the same place, which is the worse kind.

You were exactly right about the mechanism: TryGetInt64 returns false for a non-integer token as well as for an overflow, so choosing the bound by sign was right for the vast case and silently turned analysis_timeout_seconds: 30.0 into 600 with nothing added to Problems. Relocating #2444's failure into the fix for it would have been worse than leaving the original alone.

Reading the remainder as a double tells all three cases apart in one step: vast values fall outside any bound and land on one, fractional values truncate into range. The long overload compares rather than clamps, because a bound near long.MaxValue does not survive a round trip through a double.

On the suggestion to Reject a fractional value outright: I kept it clamping for the readers whose caller declared a range, because truncation there is the same species of silent adjustment the clamp already is and 30.0 unambiguously means 30. The unclamped Int(fallback) has nowhere to put a number that will not fit, so it does the opposite — takes a value that is exact however it was written, and reports the rest naming which of the two problems it has ("is not a whole number" vs "is out of range"), since "holds a JSON number where a whole number belongs" is a nonsense sentence about a number.

Two tests, and both were run against the previous commit to confirm they are red there: AFractionalNumber_ReadsAsItsValue_RatherThanLandingOnTheMaximum reports 600 where it should report 30 — your example reproduced exactly — and AnUnclampedRead_TakesAnExactNumber_AndNamesWhyItRejectsTheRest reports 7 where it should report 30. Harness now runs 16 passed / 0 failed.

[claude]

Doc-comment leftover from the last refactor commit: this <summary> block ("Records this key against its reader and hands back the caller's fallback...") originally described Reject<T> (now at line 237), but a second <summary> for the new RejectAsUnusableNumber<T> was inserted directly after it instead of replacing it. The two summaries now both attach to RejectAsUnusableNumber, and Reject<T> below is left with no doc comment at all. Not a functional bug, just orphaned documentation — worth folding the first block into (or moving it above) Reject<T> at line 237.

[claude]

Reviewed. This is a C#/WPF change (no T-SQL surface), scoped to Lite/App.xaml.cs and the new PerformanceMonitor.Common/Services/SettingsValueReader.cs.

Correctness — The core fix (per-key shape checking instead of one giant try) is sound, and I traced the numeric-edge-case logic carefully since it went through a couple of self-corrections in the PR's own commit history (Int64 boundary → sign-based clamp → the double-based fix that lands in the final diff). The final Int/Long/Double readers correctly distinguish "too big/small" (clamps to bound, silent) from "fractional" (truncates into range for clamped readers, reported as not a whole number for the unclamped reader) from "wrong JSON kind" (reported, fallback kept). I didn't find a remaining case where a legitimate value gets silently corrupted.

Parity — No Darling counterpart needed here: Darling's ViewerAppSettings (Darling/PerformanceMonitor.Darling.Viewer/ViewerAppSettings.cs) round-trips through JsonSerializer + a Normalize() clamp step rather than the manual TryGetProperty/Get* pattern Lite used, so it was never exposed to the single-try-eats-everything defect this PR fixes. No drift introduced.

Test coverage — SettingsValueReadTests.cs and the extended SettingsSampleTests.cs guard both the behavioral fix (a bad key costs only itself) and the fragile part of the design (the TryGetProperty method name being load-bearing for the regex-based key extractor). Good self-awareness in the tests about why they exist.

Left one inline nit: a leftover/duplicated XML doc comment in SettingsValueReader.cs (orphaned <summary> that used to document Reject<T>, now stranded above RejectAsUnusableNumber<T>, leaving Reject<T> undocumented). Cosmetic only, not blocking.

No security, injection, or performance concerns — this is local file/JSON parsing with no network or process boundary involved.

[claude]

Reviewed. This PR is C#-only (Lite + PerformanceMonitor.Common) — no T-SQL touched, so the T-SQL style section doesn't apply here.

Correctness: Traced every SettingsValue.Int/Long/Double/Bool/Text branch (unclamped, clamped-int, clamped-long, double) against the new SettingsValueReadTests, including the Int64-boundary and fractional-truncation cases the PR's own commit history shows were caught and fixed mid-branch. The ternary chains check out, and AlertExcludedDatabases's element-kind filter correctly replaces the old elem.GetString() throw-on-non-string-element without changing behavior on well-formed input. Verified every App.* property type (long AgRedoQueueAlertKb, double AnalysisNotifySeverity, plain int fields) matches the reader method used at its call site.

Compile sanity: s_badSettingValues.Contains(problem.ToString(), StringComparer.Ordinal) in App.xaml.cs looked at first like it needed System.Linq (not explicitly imported in that file) — but Lite/Mcp/GlobalUsings.cs has global using System.Linq;, which applies project-wide, so this resolves fine. Flagging that I checked it in case it's useful context for reviewers less familiar with the global-usings split across Lite/GlobalUsings.cs and Lite/Mcp/GlobalUsings.cs.

Security: No secret values (webhook URLs, SMTP password) ever reach SettingsReader — they're excluded and loaded via Credential Manager, so the new problem dialog/log can't leak one. The one place a raw value is echoed (RejectAsUnusableNumber, truncated to 40 chars) only fires for numeric-shape fields, never for string/secret ones.

Lite/Darling parity: Darling's ViewerAppSettings loads via whole-object JsonSerializer deserialization — a different mechanism from Lite's per-key TryGetProperty reads — so it doesn't share this defect the same way. The PR body explicitly calls this out as a deferred follow-up rather than silently leaving it, so I'm not flagging it as parity drift from this PR.

Extraction guard: Confirmed the new SettingsSampleTests self-tests can't be poisoned by doc-comments in the touched files — ReaderSources only scans App.xaml.cs/McpSettings.cs, and the new prose deliberately avoids the literal TryGetProperty(" pattern the regex keys on.

No blocking issues found. This is a solid, thoroughly self-tested fix for the "one bad value costs the rest of the file" defect.

[erikdarlingdata]

The orphaned-doc inline on SettingsValueReader.cs is already fixed on the branch — DocCommentHygieneTests caught it first and it is genuinely the funniest way this could have gone, since I was widening that exact rule on #2450 at the time.

Cause and lesson worth writing down: I anchored the insert on private T Reject<T>(...), the signature line, which put the new helper between Reject's doc block and Reject. XML docs take the last block, so tooling rendered RejectAsUnusableNumber correctly and Reject silently lost its documentation to a paragraph about a different method. Anchor C# inserts on the previous member's closing brace, not on the next member's signature.

The helper moved below rather than the stray text being deleted — Reject's block is the record of why the message is phrased the way McpSettings.Load phrases it, and #1745 found that seven of eight instances of this shape were a displaced block whose real member had been left undocumented, so a blind "remove the extra summary" would have destroyed it here too.

Both branches are rebased onto dev (8f69243) and all six checks are green on each.