Repository navigation
Drain the review-bot comments left undispositioned across #1824-#1885 - #1895
Conversation
Ten inline review comments on this campaign's PRs were never explicitly answered. Each was verified against dev first; all ten were still live. Fixed here: - install-darling.ps1 swallowed a failed service-account lookup and fell back to the virtual account. On an UPGRADE that silently re-ACLs darling.json around the wrong principal, which is the lockout the lookup exists to prevent. sc.exe qc is now a non-WMI second opinion, and an unresolvable account fails loudly on the upgrade path while a fresh install (where the default is provably right) only warns. - Four remaining docs still recommended or legitimized SQLAgentReaderRole after #1823 disproved it, including copy-pasteable SQL in the deprecated Dashboard's least-privilege script. - AlertValueParser truncated grouped numbers to the first digit group ("1,234" -> 1; de-DE "1.234" likewise). Whole groups are now read, strictly: a comma-joined list must not fuse, which .NET's own AllowThousands does not guarantee. - Duplicate InternalsVisibleTo Lite.Tests in PerformanceMonitor.Ui.csproj. - Lite's AlertBlockingWaitSecondsBox was missing from all three sibling call sites Darling got. Writing the drift guard for it found a second live instance, AlertLongRunningQueryMaxResultsBox, fixed alongside. - Lite's MCP get_alert_settings keys now match Darling's (count_threshold, alerts_enabled). Breaking for MCP clients; noted in the CHANGELOG. - DataRootMigration could not tell a migrated artifact from a placeholder recreated at the target after a failed move, stranding real data in the folder Setup.exe deletes. Empty targets are now replaced; populated ones keep the live copy and the legacy one is quarantined out of the install directory. - "bare bare" typo in BlockedProcessReportCollector. - ComposeSourceRouterTests gained the missing ReachesFurtherBack case. - Two adjacent nits: the fully-qualified AlertValueParser call sites in both history stores. Filed rather than fixed, both needing a design decision: - #1891 - Add Server dialogs clamp to the primary monitor's work area. - #1892 - Query Store time-slice filters on collection_time but buckets on interval_start_time_utc. Every new pin was red-watched: mutating each production change turns its own test red and nothing else's. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…esidue # Conflicts: # CHANGELOG.md
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /* alerts_enabled, not notifications_enabled: Darling reports the master switch under this | ||
| name, and it spells notifications_enabled something else entirely (the analysis | ||
| sub-object's own toggle), so the old Lite name was not merely different — it collided. */ | ||
| alerts_enabled = App.AlertsEnabled, |
There was a problem hiding this comment.
This is documented as a breaking change for MCP clients (notifications_enabled→alerts_enabled, threshold_count/threshold→count_threshold), and Darling's equivalent (DarlingMcpAlertTools) has a dedicated DarlingMcpAlertToolsTests.cs asserting its JSON shape. There's no equivalent test file for Lite's McpAlertTools.GetAlertSettings — nothing pins the new key names here, so a future edit could silently reintroduce the old spelling (or a new one-off) with no test catching it. Worth adding at least a smoke test asserting the top-level and nested key names Lite now emits, mirroring DarlingMcpAlertToolsTests.
Review summaryReviewed the diff for PR #1895 (drains 10 outstanding review-bot comments across #1824–#1885). Overall this is careful, well-tested work — each fix ships with a targeted regression test, and the mutation/"red-watch" testing described in the PR body is genuine (verified the Correctness spot-checks (no issues found):
One gap flagged inline: Lite's No security, missing-index-DMV, or other parity issues found beyond the one noted above. |
Review feedback on this PR: the three MCP keys renamed here are documented as breaking, and Darling's DarlingMcpAlertToolsTests has asserted its JSON shape all along, but nothing held Lite's. A rename with nothing holding it is a rename that comes back. Asserts the new spellings present AND the old ones absent - presence alone would pass just as happily if someone re-added the old key beside the new one, which is the likelier accident, and two keys meaning the same thing is its own bug for an MCP client. Runtime rather than source-parsing, since JsonOptions' naming policy means the C# identifier is not automatically the wire key. Red-watched: reverting alerts_enabled and count_threshold turns exactly the two matching tests red. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…esidue # Conflicts: # CHANGELOG.md
| log($"Data root '{newRoot}' is already populated and is the one in use, so the older copy in " + | ||
| $"'{legacyRoot}' cannot go live. Moved it to '{Path.Combine(newRoot, RescuedDirName)}' so " + | ||
| $"the next Setup.exe cannot delete it ({string.Join(", ", rescued)}). If the data you " + | ||
| "expected to see is in there rather than in the live store, close Lite and swap it in by " + | ||
| "hand; otherwise the folder is safe to delete."); |
There was a problem hiding this comment.
Misleading log message when every rescue attempt in this branch fails.
This log(...) call fires unconditionally after the foreach loop, regardless of whether any items actually landed in rescued. If TryRescue fails for every pending item (e.g. all the rescue targets already exist from an earlier partial run, or the quarantine dir can't be created), rescued is empty and this message still says:
"...Moved it to '...' so the next Setup.exe cannot delete it ()."
— with an empty parenthetical — directly followed by the kept.Count > 0 block saying "Could not move out of '{legacyRoot}': ...". The two messages contradict each other in exactly the scenario this rescue mechanism exists to make legible to the user (their data is about to be deleted by the next Setup.exe run).
Compare with the equivalent branch further down (~line 245), which correctly gates the "rescued" log behind if (rescued.Count > 0). This block should do the same, e.g.:
if (rescued.Count > 0)
{
log($"Data root '{newRoot}' is already populated and is the one in use, so the older copy in " +
$"'{legacyRoot}' cannot go live. Moved it to '{Path.Combine(newRoot, RescuedDirName)}' so " +
$"the next Setup.exe cannot delete it ({string.Join(", ", rescued)}). If the data you " +
"expected to see is in there rather than in the live store, close Lite and swap it in by " +
"hand; otherwise the folder is safe to delete.");
}No test currently exercises the all-rescue-fails path, so this wasn't caught by the new Migrate_QuarantinesTheStaleCopy_... test (which only covers the all-succeed case).
| launch and writes the marker then, and a `kept` artifact is still sitting in there. */ | ||
| if (failed.Count == 0 && kept.Count == 0) | ||
| { | ||
| TryWriteMarker(legacyRoot, newRoot, moved, log); |
There was a problem hiding this comment.
Minor: the final DATA-MOVED.txt signpost (written here via TryWriteMarker(legacyRoot, newRoot, moved, log)) only lists moved items, not rescued ones. In a mixed run (e.g. monitor.duckdb moves normally while config\ collides and gets quarantined via TryRescue), the persistent marker file will say only monitor.duckdb was moved and omit config entirely — even though config also left the legacy root and now lives under newRoot\recovered-from-install-dir\. The console log() call above (line ~245) does mention it at runtime, but that's not persisted anywhere for someone who checks the marker file later instead of the install-time log. Not data-loss (nothing here is destructive), just an incomplete signpost — might be worth including rescued in the marker text too, or noting the quarantine folder's existence in it.
Review summaryWent through the diff file-by-file and cross-checked the Lite/Darling parity claims made in the PR description against both apps' actual source (not just the changelog prose). Verified as correct
Issues found (posted inline)
Not flaggedNo missing-index DMV suggestions per your instructions, and no other Lite/Darling parity gaps found — the ten dispositions in the PR description all matched what's actually in the diff and in the current source for the app said to already be correct. |
Both are mine, both from the #1842 rescue path added earlier in this branch. The "moved it to the quarantine" log fired unconditionally after the rescue loop, so a run where EVERY rescue failed still claimed the move - naming an empty list and pointing at an empty folder. This log is the only channel that early, and an operator who reads it and finds nothing there stops looking, with their data still in the folder Setup.exe deletes. Each arm is now gated on its own list, and the all-failed case says so plainly. The DATA-MOVED.txt marker listed only `moved`, so a mixed run - store migrates, config collides and is quarantined - left a permanent signpost naming the store and omitting config entirely. Someone reads that file precisely because they are hunting one artifact by name, so the omission reads as proof it was never touched. Rescued names now appear under their own heading with the quarantine path, and the already-populated branch (where nothing goes live) writes them there rather than mislabeling them as moved. Red-watched: restoring the unconditional log and the moved-only marker turns exactly the two new tests red. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…esidue # Conflicts: # CHANGELOG.md
…esidue # Conflicts: # CHANGELOG.md
Review: #1895 — Drain the review-bot comments left undispositionedThis is a well-scoped, well-tested PR: 23 files, mostly small targeted fixes plus solid new test coverage ( Verified correct:
Two findings below (one narrow correctness gap, one signpost-completeness gap), both posted inline. Neither blocks — they are refinements to already-good defensive code. No SQL Server missing-index recommendations here as requested — this PR does not touch that surface anyway. |
| /* Only claim the move in the marker once nothing is still behind — a partial run retries next | ||
| launch and writes the marker then. */ | ||
| if (failed.Count == 0) | ||
| launch and writes the marker then, and a `kept` artifact is still sitting in there. Rescued | ||
| names go in too, under their own destination: they also left this folder, and a marker that | ||
| named only `moved` would tell someone hunting for their settings.json that it was never | ||
| touched. */ | ||
| if (failed.Count == 0 && kept.Count == 0) | ||
| { | ||
| TryWriteMarker(legacyRoot, newRoot, moved, log); | ||
| TryWriteMarker(legacyRoot, newRoot, moved, rescued, log); | ||
| } |
There was a problem hiding this comment.
Correctness (signpost completeness across multiple launches): moved/rescued/kept are local to a single Migrate() call, and the marker is only written when kept.Count == 0 (same gating exists in the AlreadyMigrated branch above at ~190-195). That's correct for a single run, but across multiple runs it can silently drop history from the marker:
- Launch 1: artifact A rescues successfully, artifact B's rescue fails (
kept = [B]). Per the "claiming a move that didn't happen is worse than saying nothing" rule, the marker is (rightly) not written this run. - Launch 2: A is no longer in
legacyRoot(it already moved), sopendingonly contains B. B now succeeds. This run's localrescued = [B], and sincekept.Count == 0now,TryWriteMarkerfires — but with a list that only contains B. - Once written, the marker is never revisited (a later launch finds
pending.Count == 0and returnsNothingToMigratebefore reaching any marker logic).
End state: A really did move to RescuedDirName and is safe, but DATA-MOVED.txt never mentions it — only B. That directly undercuts the rationale given a few lines up ("a marker naming only the artifacts that went live would read... as proof it was never touched at all") and in the TryWriteMarker doc comment, just for a multi-launch retry instead of a single-launch partial failure. Worth accumulating moved/rescued names that leave the legacy root even on runs where the marker isn't written yet (e.g., append to a running list persisted via a sidecar, or just always write/update the marker with whatever has left so far, rather than gating the entire marker on kept.Count == 0).
| if (!seenSeparator && IsGroup(span, end, groupSeparator)) | ||
| { | ||
| end += groupSeparator.Length + GroupSize; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Correctness (minor/edge-case): IsGroup validates that a group following a separator is exactly 3 digits, but nothing bounds the length of the leading digit run before the first separator is seen — the digit-consuming branch a few lines up (char.IsAsciiDigit(span[end])) will happily consume 4+ digits before ever reaching a separator.
That means a malformed leading run followed by one well-formed group is accepted as a single continuous number, even though it isn't valid grouped formatting: "1234,567" (en-US) — leading run "1234" (4 digits, not ≤3), then ,567 which does pass IsGroup (exactly 3 digits, then end-of-string) — so the whole token "1234,567" goes to double.TryParse with AllowThousands, which (per the doc comment on IsGroup, citing double.Parse("1,2,3", en-US) == 123) doesn't validate placement either, and returns 1234567.
Before this PR, that same input would have stopped at the first non-decimal separator and returned 1234 — so this is a new (if narrow) way to fuse two adjacent numbers, which is exactly what the PR's own stated goal ("a comma-joined list must not fuse into one number") is trying to prevent. It's not covered by the new test cases, which only exercise malformed trailing groups ("1,2,3", "1,2345"), not a malformed leading run followed by a valid group.
Given no current producer emits this shape, practical impact is low today — but since the whole point of IsGroup is documented strictness, it's probably worth bounding the leading run to 1–3 digits too (track how many digits were consumed before the first separator and reject if it's not 1–3).
The marker file was assembled from the calling run's moved/rescued lists, which are local to one Migrate() call. Launch 1 rescues A and fails on B, so it correctly writes nothing; launch 2 sees only B pending, succeeds, and writes a marker naming B alone - with A safe in the quarantine and never mentioned again, because a later launch finds nothing pending and returns before any marker logic. The lists are now read off the filesystem at write time, which has no such hole and carries no state between runs, and the marker names what is STILL in the folder too - so it can be written as soon as anything leaves instead of waiting for a clean run. AlertValueParser bounded the digits AFTER a group separator to exactly three but never the leading run before the first one, so "1234,567" fused into 1234567 - a NEW way to join two adjacent numbers, which is the exact thing this PR's strictness exists to prevent, and worse than the 1234 it returned before grouping was handled at all. The leading run must now be a valid first group (1-3 digits). Only the first separator needs it; every later run is exactly three by construction. Plain decimals like "1234.56" are untouched - the bound is on groups, not on digits. Red-watched: reverting either turns exactly its own tests red. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…esidue # Conflicts: # CHANGELOG.md
| GRANT SELECT ON dbo.sysjobs TO [SQLServerPerfMon]; | ||
| GRANT SELECT ON dbo.sysjobactivity TO [SQLServerPerfMon]; | ||
| GRANT SELECT ON dbo.sysjobhistory TO [SQLServerPerfMon]; | ||
| GRANT EXECUTE ON dbo.agent_datetime TO [SQLServerPerfMon]; |
There was a problem hiding this comment.
Nit: agent_datetime isn't actually referenced by any of the Full Edition's T-SQL collectors (install/*.sql) — it's only used by PerformanceMonitor.Collectors/AgentStatusCollector.cs, which is Lite/Darling-only (not wired into the deprecated Dashboard). sysjobs, sysjobactivity, and sysjobhistory are the three actually read by install/51_collect_running_jobs.sql / 44_hung_job_monitor.sql, matching that file's own dependency comment exactly. Harmless to grant (trivial EXECUTE on a system scalar function), but since this section's whole point is precision over the old blanket SQLAgentReaderRole recommendation, this one grant doesn't belong in the "least privilege" recipe for this SKU.
ReviewWent through the full diff (23 files, +945/-88) against the ten dispositions the PR body lists, cross-checking each claim against the actual current source rather than just the PR description. Summary: this holds up well — I traced the trickiest piece of new logic end-to-end and didn't find a defect, and the Lite/Darling parity claims check out against the live code on both sides.
Lite/Darling parity — verified directly against install-darling.ps1 — the fresh-vs-upgrade split on account-resolution failure (warn-and-proceed vs. One minor doc nit, left inline: the corrected Nothing else stood out — no SQL injection surface (no T-SQL changed in this PR), no secrets handling issues, no performance regressions in the changed paths. The CHANGELOG entries I spot-checked against the code match what actually shipped. |
Review feedback on this PR. Replacing SQLAgentReaderRole with direct table grants, I copied the recipe from the Lite/Darling README rather than from this SKU's own collectors. Verified against the T-SQL: agent_datetime is never referenced by install/*.sql at all - it is AgentStatusCollector.cs, which the Dashboard does not even reference - so it does not belong in a least-privilege recipe whose whole point is precision. The check also turned up the opposite error the review did not mention: 51_collect_running_jobs.sql reads msdb.dbo.syssessions, which my grant block omitted, so following it would have left the collector failing on the one table nobody listed. The four granted now are exactly the four those two scripts read. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| /* Direct table grants, deliberately NOT SQLAgentReaderRole: that role gates the sp_help_job* | ||
| procedures, which this product never calls, and grants NO SELECT on the tables the running | ||
| jobs collector actually reads - with only the role, those reads fail with error 229 (#1823). | ||
| These four are exactly what 51_collect_running_jobs.sql and 44_hung_job_monitor.sql read. | ||
| Lite and Darling need a wider set (syscategories, sysjobschedules, and EXECUTE on | ||
| agent_datetime) for collectors this edition does not have - see the main README. */ | ||
| USE [msdb]; | ||
| CREATE USER [SQLServerPerfMon] FOR LOGIN [SQLServerPerfMon]; | ||
| ALTER ROLE [SQLAgentReaderRole] ADD MEMBER [SQLServerPerfMon]; | ||
| GRANT SELECT ON dbo.sysjobs TO [SQLServerPerfMon]; | ||
| GRANT SELECT ON dbo.sysjobactivity TO [SQLServerPerfMon]; | ||
| GRANT SELECT ON dbo.sysjobhistory TO [SQLServerPerfMon]; | ||
| GRANT SELECT ON dbo.syssessions TO [SQLServerPerfMon]; |
There was a problem hiding this comment.
This grant block is still incomplete for 44_hung_job_monitor.sql. That script's @stop_hung_job parameter defaults to 1, and when a hung job is detected it runs EXECUTE msdb.dbo.sp_stop_job (install/44_hung_job_monitor.sql:195-196) — a job-control EXECUTE, not a table SELECT. None of the four GRANT SELECTs here cover it, and it's not listed under "Optional" either.
Under this exact least-privilege recipe, the default hung-job auto-stop will fail its permission check every time it fires — caught and logged to collection_log rather than crashing, but silently non-functional despite an operator following the doc precisely. This is the same class of gap the commit message for this change says it went and verified against the T-SQL (which is how syssessions got added) — sp_stop_job just wasn't the miss that pass caught.
| } | ||
|
|
||
| if (moved.Count == 0 && failed.Count == 0) | ||
| if (moved.Count == 0 && rescued.Count == 0 && failed.Count == 0) |
There was a problem hiding this comment.
This early return can silently strand data with zero log output — the exact failure mode this PR otherwise guards against.
If every pending artifact collides with real content at the target and TryRescue also fails for all of them (e.g. recovered-from-install-dir\ already has an entry with that name from an earlier partial run, or the rescue move throws), then moved, rescued, and failed all stay empty while kept is not. This condition is true, so the method returns AlreadyMigrated immediately — skipping the moved > 0 / rescued > 0 / kept > 0 log blocks a few lines below and skipping TryWriteMarker entirely.
Per this method's own doc comment, "log is the only channel out" this early — so the user gets no indication that real data (e.g. config/settings.json) is still sitting in the folder Setup.exe will delete on the next upgrade.
This is exactly the scenario the PR added a dedicated log + test for in the sibling HasCompleteStore(newRoot) branch above it (Migrate_EveryRescueFails_SaysSoInsteadOfClaimingAMove), but this per-item-loop path has no equivalent coverage. Reproduces with a single pending item that collides with real (non-placeholder) content and whose rescue also fails.
| Status = failed.Count > 0 ? DataRootMigrationStatus.PartiallyMigrated : DataRootMigrationStatus.Migrated, | ||
| Moved = moved, | ||
| Kept = kept, | ||
| Rescued = rescued, |
There was a problem hiding this comment.
Minor inconsistency: the outer catch (Exception ex) block a few lines below this method (not itself touched by this PR, so it doesn't show in the diff) builds its DataRootMigrationResult with Moved, Kept, Failed but no Rescued = rescued. If an artifact was already moved into recovered-from-install-dir\ before a later exception aborts the run, the result reports Rescued as empty even though the rescue actually happened — unlike this return (and the AlreadyMigrated one above), which both now populate Rescued. App.xaml.cs doesn't currently read result.Rescued, so it's not user-visible today, but it's a real gap in the contract this PR just introduced.
|
Review summary This PR dispositions ten review-bot comments left open across #1824-#1885: eight fixed here, two filed as new issues (#1891, #1892). It's unusually disciplined about it — each fix is red-watched (a targeted mutation confirmed to turn only its own test red), and three of the five commits are the author fixing bugs a self-review found in this PR's own earlier commits (the marker-log claiming a move that didn't happen, the marker not surviving multiple launches, the grouped-number parser fusing "1234,567", and the deprecated Dashboard's grant list missing Scope checked: correctness/edge-cases, Lite/Darling parity, security (the What holds up well
Issues found (posted inline)
Nothing else stood out as a correctness, parity, or security issue in the diff. |
…stop_job All three verified against the code before acting; one of the three claims turned out to be overstated and the fix is scoped to what is actually true. The per-item loop's early return ran before the summary log blocks, so a run where every artifact collided AND every rescue then failed returned without the line telling the operator these are still in the folder Setup.exe deletes. NOT total silence as the review had it - TryRescue logs its own per-item failure either way - but the actionable summary, the only line that names the whole set and says what to do, was skipped. Logging now precedes every return. The first version of the test asserted only that SOMETHING was logged and passed with the bug still in place; it asserts the summary line specifically now, and red-watches correctly. The outer catch built its result without Rescued, so an artifact already quarantined before a later throw was reported as not rescued. It also could not reach PartiallyMigrated on rescues alone. Both corrected - a rescue is progress exactly like a move. 44_hung_job_monitor.sql runs EXECUTE msdb.dbo.sp_stop_job when it finds a hung job, and @stop_hung_job defaults to 1, so following the least-privilege recipe as written left the auto-stop failing its permission check on every fire. Documented as a conditional grant with the alternative (@stop_hung_job = 0) spelled out, since it is job CONTROL rather than monitoring and not everyone wants it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…esidue # Conflicts: # CHANGELOG.md
ReviewWent through the full diff (23 files) plus the current state of the touched code paths. This is a disciplined audit PR — each of the 8 fixed items is scoped tightly to its issue, backed by tests that were mutation-checked (confirmed via the "red-watch" table), and the two deferred items are filed rather than rushed. Nothing here reads as scope creep. Verified correct by manual trace (not just trusting the new tests):
One finding — a leftover instance of the exact bug this PR fixes:
This is the XML summary doc comment on This PR's #1826 fix explicitly frames itself as "a repo-wide sweep for the same shape" that "found a fourth [instance] that matters more than the other three combined." Given that framing, this is a fifth instance in the same file as an instance the PR already fixed (the (Lower-confidence, optional: No correctness bugs, no Lite/Darling drift, and no security issues found beyond the one doc-comment leftover above. Didn't flag any missing-index DMV suggestions per your standing preference. |
Fix the three #1895 residue items: per-monitor dialog clamp, Query Store window edges, cpu.mode vocabulary
erikdarlingdata#1892, erikdarlingdata#1911 erikdarlingdata#1891 - the Add Server dialogs clamped against SystemParameters.WorkArea, which always reports the PRIMARY monitor. On a secondary monitor shorter than the primary that reproduced the very off-screen-footer bug erikdarlingdata#1828/erikdarlingdata#1829 fixed. The clamp and the height cap now resolve the actual monitor via MonitorFromWindow + GetMonitorInfo, converted from physical pixels to WPF units through the window's own presentation source - an unconverted work area on a 150%-scaled screen would be a worse bug than the one being fixed. Re-evaluated on SourceInitialized and LocationChanged, because the cap it replaced was a one-shot XAML binding read once at load, so a dialog dragged to a shorter screen kept the taller screen's cap forever. Every path that cannot get a real answer falls back to the old behaviour, so single-monitor is unchanged. Win32 rather than WinForms: neither app references WinForms and one lookup is not worth a second UI framework. Shared between both apps so the twins cannot drift. erikdarlingdata#1892 - the Query Store slicer and duration trend filtered the window on collection_time while keying the bars on the interval start. Once erikdarlingdata#1841 made those different instants the chart disagreed with its own range at both edges: an interval starting before the range but collected inside it drew a bar dated before the range began, and the range's final interval - still open when the range ended - was dropped entirely. Both reads now window on the expression they place on, in both apps. A bare collection_time floor stays and is NOT a filter: query_store_stats is a hypertable partitioned on that column and Lite's view unions the parquet archive, so without it Postgres decompresses every chunk and DuckDB cannot skip row groups. An interval is always collected after it starts, so the floor is provably implied and can never decide membership - the same shape as the drill-down collector's last_execution_time bound. Deliberately no upper twin: collection_time can exceed the interval start without bound if the collector was down when the interval closed, so a ceiling would silently drop exactly the rows a restarted collector just caught up on. erikdarlingdata#1911 - Lite's MCP get_alert_settings did not report cpu.mode at all. Reporting the raw enum name would have traded erikdarlingdata#1895's key-level mismatch for a value-level one, since Darling emits "sql"/"total" and validates writes against exactly those. Darling's vocabulary wins as the older public surface; the cross-app literals are pinned by reading Darling's own source, because asserting Lite's constants alone would keep passing the day Darling changed. Verified live against PostgreSQL 18.4 + TimescaleDB 2.28.1 (4002 passed, 10 skipped - the gated live suite genuinely ran). Every new pin red-watched, including the live one. The erikdarlingdata#1902 ratchet correctly caught the new live test using the old teardown idiom; converted to LiveStoreCleanup rather than raising the ceiling. Filed rather than folded in: erikdarlingdata#1921 (the item-timeline overlay still plots at collection_time while the bars it annotates are keyed on the interval start - a product decision about what the overlay is for) and erikdarlingdata#1922 (the live fixture keeps using a connection that CREATE EXTENSION timescaledb killed, masking the real cause). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Ten inline review-bot comments across #1824-#1885 were never explicitly dispositioned. Each was verified against
origin/devbefore being touched - all ten were still live; none had been fixed by a later PR. Eight are fixed here, two are filed.Disposition
Get-CimInstancecatch swallows a failed service-account lookupSQLAgentReaderRoleWorkAreais primary-monitor-onlyAlertValueParsertruncates grouped numbersInternalsVisibleTo Lite.TestsAlertBlockingWaitSecondsBoxmissing from 3 sibling call sitesDataRootMigrationcannot tell a migrated artifact from a stray onebare baretypoReachesFurtherBackbranch untestedWhat each fix does
#1824 - the installer stops guessing which account it is hardening for. The service-account lookup exists so an upgrade cannot strip the grant of a domain account or gMSA an operator re-homed the service to. Its
catchwas silent, so a WMI failure fell back to the virtual account and reproduced exactly that lockout with nothing on screen.sc.exe qcis now tried as a non-WMI second opinion (a broken WMI repository alone should not derail an upgrade), and if the account still cannot be resolved the paths split: fresh install warns and proceeds (sc createjust set that account, so the default is provably right), upgrade stops with an actionable error rather than ACLing a principal it had to guess.#1826 - four stale
SQLAgentReaderRolerecommendations. The bot named three; a repo sweep found a fourth that matters more than the other three combined - the deprecated Dashboard's least-privilege section still hands outALTER ROLE [SQLAgentReaderRole] ADD MEMBERin copy-pasteable SQL, i.e. instructions #1823 proved do not work. All four corrected, plus the doc comments inFailedJobsQuery,DarlingWorker,AlertHealthResultandDatabaseService.NocHealththat described the role as the thing whose absence causes the skip.#1834 - grouped numbers. Survey first: no
CurrentValueTextproducer formats with grouping today, so the bot's "doesn't fire yet" holds. ButAlertContextBuilders- alert code one file over - fills its detail fields with:N0, so "nobody groups" is a habit, not a guarantee, and the de-DE case is worse than the en-US one because there the group separator IS.. Chose to handle it rather than document a non-goal: the failure mode is a plausible-looking wrong number, which is strictly harder to notice than the: 0this parser replaced.The handling is strict on purpose. .NET's own
AllowThousandsdoes not validate group placement -double.Parse("1,2,3", en-US)returns 123 - so handing the token toTryParseand trusting it would fuse any comma-joined list. A group is accepted only when the separator is followed by exactly three digits and then a non-digit, so"1,234"reads as 1234 while"1,5","55,66"and"1,2,3"stop at the first run exactly as before.#1840a - and a second instance of the same miss. The three sibling call sites are wired to match Darling. Building the drift guard for them surfaced
AlertLongRunningQueryMaxResultsBox, wired into neitherRestoreAlertDefaultsButton_ClicknorUpdateAlertControlStatesin Lite and into both in Darling - the identical defect one control over, fixed alongside. Both apps are now pinned byAlertSettingsControlWiringTests: everyAlert*Boxa settings window LOADS must also be gated and reset. Load and save are impossible to forget (the box does nothing without them); the other two fail silently, which is why this is the pair worth pinning.#1840b - MCP keys. Breaking for clients reading Lite's payload.
blocking.threshold_countbecomescount_threshold,deadlocks.thresholdbecomescount_threshold, and top-levelnotifications_enabledbecomesalerts_enabled. Darling's spelling wins as instructed. The third rename was not in the bot's comment and is the one worth flagging: Darling spellsnotifications_enabledsomething else entirely (the analysis section's own toggle), so Lite's name was not merely different but collided - an agent reading that key got two different meanings depending on which app answered. Fixing one key and leaving that would have shipped a known second mismatch. The #1839 CHANGELOG entry, which documented the oldthreshold_count, is corrected in place - same release, so it must not contradict the code.#1842 - the biggest one. The bot's argument holds and the trigger is a single transient lock. If
monitor.duckdbfailed to move once, the app opened DuckDB at the new root and a fresh empty database appeared there the same session; from the next launch onHasCompleteStoreread as done and the real store was never looked at again. Theconfig\variant is likelier still: startup runsConfigSeederregardless of whether the migration failed, so a failedconfig\move leaves a seeded stand-in permanently occupying the target while the user's realsettings.jsonwaits in the install directory forSetup.exeto delete it.Note that the empty-directory/zero-byte refinement alone does not fix either case - a fresh DuckDB file is not zero bytes and a seeded config directory is not empty. So it is implemented as the narrow half plus a durable one:
recovered-from-install-dir\under the data root, whereSetup.execannot reach it.archive\from an older install cannot quietly become the live one.Deliberately not attempted: swapping a bigger legacy store in for a live one. Size is no evidence of which one the user wants. The decision stays theirs - the data just stops sitting somewhere deletable while they make it.
Testing
dotnet build PerformanceMonitor.sln -c Debug -t:Rebuild- Build succeeded, 0 Warning(s), 0 Error(s).Installer.Tests deliberately not run.
install-darling.ps1validated with the PowerShell language parser - parses clean.Red-watch. Every new pin was mutated to confirm it is not vacuous, and each mutation turned only its own test red:
ReachesFurtherBackfromPreferDayGrainDailyQueryStore_NeitherDailyCoversTheWindow_DeeperDayGrainStillWinskeptshort-circuit inMigrateAlertBlockingWaitSecondsBox.IsEnabledfrom LiteReview rounds on this PR
The bot found six more, all against code this PR introduced. Every one is fixed here, each verified against the source before acting, each red-watched.
DarlingMcpAlertToolsTests)McpAlertSettingsKeyTests- new names present AND old ones absent, since re-adding the old key beside the new one is the likelier accidentDATA-MOVED.txtlisted only what went live, omitting quarantined artifactsIsGroupbounded digits after a separator but not the leading run, so"1234,567"fused into 1234567catchomittedRescuedRescuedpopulated and counted towardPartiallyMigratedTwo of the six are worth calling out because I did not simply take the reviewer's word:
agent_datetimegrant, the review was right that it belongs to a collector this SKU does not have - and checking the T-SQL myself turned up the opposite error it did not mention:51_collect_running_jobs.sqlreadsmsdb.dbo.syssessions, which my grant block omitted. Following the recipe as written would have left the collector failing on the one table nobody listed. A later round then caughtsp_stop_job(44_hung_job_monitor.sql,@stop_hung_jobdefaults to 1), documented as a conditional grant with the@stop_hung_job = 0alternative, since it is job control rather than monitoring.TryRescuelogs its own per-item failure either way. What was actually skipped is the summary line that names the whole set and says what to do about it. My first version of that test asserted only that something was logged and passed with the bug still in place; it asserts the summary specifically now.Filed, not fixed
SystemParameters.WorkAreais primary-monitor-only in both Add Server dialogs. Needs per-monitor resolution via WinFormsScreenorMonitorFromWindow, plus a DIP conversion and aLocationChangedre-evaluation for theMaxHeightbinding, in both apps.collection_timebut buckets oninterval_start_time_utc. Assessed honestly: no clean clamp exists. The three options (accept and document / filter on the bucket key / widen-and-clamp) each have a real cost, and the middle one interacts with the dedup CTE'sORDER BY collection_time DESC. Written up with the edge documented.🤖 Generated with Claude Code