The Darling sidebar dot and the Overview card disagreed about a never-collected server (#2473) - #2474
Merged
Merged
Conversation
…ered differently On dev, a registered-but-never-collected server draws a grey "Unknown" dot in the sidebar and an amber "Awaiting first collection" card one panel over. Both are stamped from the SAME ClassifyFreshness call. Verified before touching anything, and the premise holds exactly as #2473 describes it: DarlingServer.ApplyFreshness sets two flags where ServerSummaryItem.ApplyFreshness sets three, DarlingServer has no AwaitingFirstCollection to set, and MainWindow.xaml carries DataTriggers for Online/Offline/Warning only -- so the fifth state fell through to ForegroundMutedBrush and painted grey without failing anything. The dot is the thing a reader points at first, and it was silently giving the pre-#2429 answer. WHAT THE DOT NOW SAYS, AND WHY Amber, matching the card, with the card's word. The card already chose amber for this state and wrote down why -- a queued server is not a dead one, which is the 24-server field incident #1552 answered. Grey is arguably honest ("nothing is known yet") and it was the smaller change, but it is a SECOND answer to a question the card has already answered on the same screen, and having two answers is the entire defect. The sidebar's job here is to render the card's decision, not to hold an opinion about it. Amber now covers two states on the dot (a stale collection and a never-collected server), which is exactly what the card already does -- the card disambiguates them with a word and a dot has no room for one, so the dot gains a tooltip instead. Its first line is the shared headline, word for word what the card says. WHERE THE LADDER LIVES Not in the viewer. Collapsing the two viewer surfaces onto a viewer-local rules class would have produced a pin that could not span the files holding the other copies, and #2470's whole lesson is that a pin scoped shorter than the duplication is worth nothing. Looking for those files turned up two more: - DarlingFleetReader.StatusLabel -- the web/MCP fleet card's word. Agrees with the card today; a fourth independent derivation of the same five states. - DarlingMcpDataTools.FreshnessStatus -- list_servers, written as if statements, in another file, against its OWN copies of the 2-minute and 15-minute thresholds. That is the #2470 evasion shape twice over, plus a real drift bug: ServerHealthThresholds could move and list_servers would go on answering with the old numbers. Both live in the headless service, which cannot reference WPF, so the only assembly all four can render from is PerformanceMonitor.Common -- where ServerHealthClassifier and the thresholds already are, and where ClassifyBand already takes this exact flag triple. The type is ServerCollectionStatus, not ServerCardStatus, and the rename is load-bearing rather than cosmetic. Lite has a ServerCardStatus of its own answering a DIFFERENT question -- its word comes from a live connection check, this one from how old the newest collection is -- and #2457 turned down folding freshness into Lite's word precisely to keep the two axes apart. Common already names that distinction (ServerConnectionStatus is the connection answer); this is the collection one. Sharing a name would have invited the conflation both issues were about, and Lite/Models/ServerConnection.cs imports both namespaces, so it would also not have compiled. WHAT THE COLLAPSE ACTUALLY IS Classify(isOnline, hasCollectorErrors, awaitingFirstCollection) is the ladder. Word(), McpToken() and Headline() are three renderings of its result -- not three ladders, which is the correction #2470 had to make once already: a second switch is a second ladder even when it returns the right type. McpToken differs from Word in exactly one arm because "AwaitingFirstCollection" shipped as a published MCP status value and clients key on it; a status value is a consumer API, so the vocabularies stay apart and sit next to each other where the difference reads as a decision. FlagsFor(ServerFreshness) is the other half, and it is the half that fixes the bug rather than tidying it. Both ApplyFreshness implementations classified freshness and then wrote the flags out by hand; the sidebar's wrote two of three and dropped the awaiting marker on the floor. Nothing about a block of assignments makes a missing one visible. They now arrive as one value. Behaviour is unchanged everywhere except the sidebar dot. The MCP list_servers tokens were checked arm by arm against the thresholds they used to duplicate (2 min stale, 15 min offline -- identical), and StatusLabel's output is byte-identical. Verified on macOS: Common, the Darling viewer, the Darling service, Lite, Lite.Tests and Darling.Tests all build. The ladder itself was run against the real build in a throwaway net10.0 console -- every freshness band through FlagsFor and Classify agrees with FromFreshness, the #2429 flag pairs still resolve the way the card always resolved them, and all four list_servers tokens match the pre-change thresholds. The WPF suites cannot run on macOS; CI is the arbiter for those. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The pin that would have caught this one has to survive the two ways #2470's missed it: the copy was in a DIFFERENT FILE and written as if statements rather than a switch, so a scan for one literal in one file lost on both axes at once. And the Darling tree turned out to hold a third variation -- a copy that spelled only four of the five words, which is a scan for the fifth word losing too. So there are two complementary scans, both across three trees (PerformanceMonitor.Common and both Darling trees), both syntax-agnostic: A. "Awaiting first collection" / "AwaitingFirstCollection" appear in code in exactly one file. Any FIVE-state copy has to spell one of them; nothing else in the product has a reason to. B. Any file that calls ClassifyFreshness spells none of the status words. That is the half that catches a FOUR-state copy -- one that never writes the fifth word and so slips past A, which is precisely what DarlingServer.DotStatus was. It also auto-includes a new file: freshness in, a word out, and the only legal path between them is the rules. "Warning" is deliberately not in B's forbidden set. It is shared with the alert-badge severity, the AG-health labels and the fleet band labels, and a text scan cannot tell those from this ladder. It costs nothing -- no copy of this ladder can be written without also spelling "Online" and "Offline". Two things make the scans mean what they say. Comments are stripped by a lexer rather than a line prefix, because three of the four copies sit in files whose doc comments legitimately quote all five words and whose block comments run mid-file; the lexer handles verbatim and raw string literals too, since a scan that mangles a literal is a scan that can miss the copy it exists to find. And B asserts it FOUND each of the four known classifiers by path, so a rename cannot leave it walking nothing and passing. XAML is out of B's scope on purpose: MainWindow.xaml has to spell the words to match on them. EveryStateTheCardPaints_HasADotColourToMatch holds those spellings to the enum instead -- it enumerates ServerCollectionStatus, reads the card's own brush for each state, and asserts the sidebar has a trigger reaching for that severity's theme brush. That assertion alone would have caught the defect: two of five states had no trigger, and a missing trigger paints the muted default and fails nothing. Unknown is asserted to have NO trigger, because "grey by design" and "grey by omission" look identical in XAML and this is the file where the difference gets recorded. The #2429 separation gets its viewer-side line too. In Darling the conflation was one amber word standing for a stale collection AND for a metric breach, so the pin is that a server whose metrics are on fire but whose collection is current is still Online, held at Classify's signature -- three flags, no severity. And the guard runs on the changes it guards, which is #2471's finding one layer out. This suite reads PerformanceMonitor.Common as well as both Darling trees, so it asserts the "Run Darling tests" step gates on core as well as darling, and that the two filters really cover those trees. They already did; the assertion is there so that stays true. Verified by simulation against the shipped files, since the WPF suites cannot run on macOS: every scan and both XAML assertions were reimplemented over the real trees, including the comment lexer. On this branch all six pass. Against dev, five of the six fail -- Part A on two files, Part B on eleven word/file pairs across three files plus the vacuity check on DarlingMcpDataTools (whose only ClassifyFreshness mention there is in a comment, which is the lexer doing its job), the trigger-coverage check on the two missing states, and the tooltip binding. Darling.Tests and Lite.Tests both build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ReviewWent through the full diff (test file, the four call-site refactors, the XAML trigger addition, and the new Correctness — verified, no bugs found:
Parity — no drift: This only touches Darling + the shared Security / perf: N/A — no external input, no new I/O, just enum-driven switch expressions. One trivial nit, not blocking: Nice pin design overall — the "spell the word" + "call the classifier without spelling a word" two-part scan is a good answer to the four-state-copy evasion #2470 already ran into once. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2473.
The premise held
Checked against
devbefore changing anything, and it is exactly as filed.ServerSummaryItem.ApplyFreshnessandDarlingServer.ApplyFreshnessstart from the sameClassifyFreshnesscall; the card sets three flags, the sidebar row sets two, andDarlingServerhas noAwaitingFirstCollectionto set.MainWindow.xamlcarriesDataTriggers forOnline/Offline/Warningonly, so the fifth state falls through toForegroundMutedBrush.A registered-but-never-collected server therefore draws a grey "Unknown" dot beside an amber "Awaiting first collection" card, one panel over, off one classification. The dot is the thing a reader points at first and it was silently giving the pre-#2429 answer.
The decision the issue asked for first
Amber, matching the card, in the card's words. The card already chose amber and wrote down why — a queued server is not a dead one, which is the field incident #1552 answered. Grey is defensible on its own terms ("nothing is known yet") and was the smaller change, but it is a second answer to a question the card has already answered on the same screen, and having two answers is the whole defect. The sidebar's job is to render the card's decision, not to hold an opinion about it.
Amber now covers two states on the dot, exactly as it already does on the card. The card disambiguates them with a word; a dot has no room for one, so the dot gets a tooltip whose first line is the shared headline, word for word.
Where the ladder went, and why it is not viewer-local
Collapsing just the two viewer surfaces would have produced a pin that could not span the files holding the other copies — and #2470's lesson is that a pin scoped shorter than the duplication is worth nothing. Looking for those files turned up two more copies, both in the headless service:
ServerSummaryItem.StatusDisplayDarlingServer.DotStatusDarlingFleetReader.StatusLabelDarlingMcpDataTools.FreshnessStatusifchain, own 2 min / 15 min thresholdsThat last one is the #2470 evasion shape twice over (different file,
ifstatements) plus a real drift bug:ServerHealthThresholdscould move andlist_serverswould go on answering with the old numbers.Both service copies live in an assembly that cannot reference WPF, so the only home all four can render from is
PerformanceMonitor.Common— whereServerHealthClassifier, the thresholds, and aClassifyBandtaking this exact flag triple already live.The rename is load-bearing
The shared type is
ServerCollectionStatus, notServerCardStatus. Lite has aServerCardStatusanswering a different question — its word comes from a live connection check, this one from how old the newest collection is — and #2457 turned down folding freshness into Lite's word precisely to keep the two axes apart. Common already names that distinction:ServerConnectionStatusis the connection answer, this is the collection one. Sharing a name would have invited the conflation both issues were about, andLite/Models/ServerConnection.csimports both namespaces, so it would also not have compiled.The collapse
Classify(isOnline, hasCollectorErrors, awaitingFirstCollection)— the ladder, once.Word()/McpToken()/Headline()— three renderings of its result, not three ladders.McpTokendiffers fromWordin exactly one arm becauseAwaitingFirstCollectionshipped as a published MCP status value and clients key on it; a status value is a consumer API. The two vocabularies sit next to each other so the difference reads as a decision.FlagsFor(ServerFreshness)— the half that fixes the bug rather than tidying it. BothApplyFreshnessimplementations wrote the flags out by hand and the sidebar's dropped one on the floor. Nothing about a block of assignments makes a missing one visible; they now arrive as one value.Behaviour is unchanged everywhere except the sidebar dot.
The separation that had to be preserved
#2457's line is Lite's: freshness stays out of a connection word. The viewer's equivalent is #2429's: one amber word must not stand for a stale collection and a metric breach.
Classifytakes three flags and no severity, and a server whose metrics are on fire but whose collection is current is stillOnline— pinned, at that signature.The pin
Two complementary scans across
PerformanceMonitor.Commonand both Darling trees, both syntax-agnostic:ClassifyFreshnessspells none of the status words. Catches a four-state copy, which is what this defect was and which A cannot see. It also auto-includes new files: freshness in, a word out, one legal path between."Warning"is deliberately out of B's forbidden set — it is shared with the alert-badge severity, the AG-health labels and the fleet band labels, and a text scan cannot tell those apart. It costs nothing: no copy of this ladder can be written without also spelling"Online"and"Offline".Comments are stripped by a lexer, not a line prefix (three of the four copies sit in files whose doc comments legitimately quote all five words), and B asserts it found each known classifier by path, so a rename cannot leave it walking nothing and passing.
XAML is out of B's scope on purpose — it has to spell the words to match on them.
EveryStateTheCardPaints_HasADotColourToMatchholds those spellings to the enum instead: it enumerates the enum, reads the card's own brush per state, and asserts the sidebar has a trigger reaching for that severity's theme brush. That assertion alone would have caught this — a missing trigger paints the muted default and fails nothing.Unknownis asserted to have no trigger, because "grey by design" and "grey by omission" look identical in XAML.Verification, and its scope
Lite.Tests,Darling.Tests.net10.0console: every freshness band throughFlagsFor+Classifyagrees withFromFreshness; the Say why the card is amber, and give "+52 more" somewhere to go (#2424) #2429 flag pairs resolve as the card always resolved them; all fourlist_serverstokens match the pre-change duplicated thresholds arm for arm. 35 checks, all pass.dev, five of six fail — A on two files, B on eleven word/file pairs across three files plus the vacuity check onDarlingMcpDataTools(whose onlyClassifyFreshnessmention there is in a comment, which is the lexer working), trigger coverage on the two missing states, and the tooltip binding.Darling.Testsruns underdarling || core || root; this suite readsPerformanceMonitor.Commontoo, so the gate is asserted rather than assumed (Give the catch-up horizon one home, because the copies of it went stale (refs #2468) #2471's finding, one layer out).🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 5 noreply@anthropic.com