The alert engine takes per-server and per-database threshold overrides.

[erikdarlingdata]

Claude posting for Erik Darling.
Fixes #5910. Part of #5632.

Why

An agent at Level 2 may tune a threshold for one server, not for the whole fleet. Until now the alert engine read every threshold as one global value. This slice gives the engine a way to ask for a per-server or per-database value. When nobody supplies one, nothing changes. No host supplies overrides yet: Lite passes none, and the Darling store-backed loader is a later slice.

What changed

Part 1: the registry (PerformanceMonitor.Alerting/AlertThresholdKey.cs)

• AlertThresholdKey: one key per tunable integer member of IAlertEngineSettings. Each key has the same name as its member.
• AlertThresholdKeys.Scopes: each key's scope, either Server or ServerAndDatabase.
• AlertThresholdKeys.NotTunable: every other member of the settings, each with a reason.
• AlertThresholdKeys.GlobalValue(settings, key): the only place a key's member is read off the settings object.

Part 2: the engine parameter (IAlertThresholdOverrides.cs, AlertEngine.cs)

• IAlertThresholdOverrides.Resolve(AlertThresholdKey key, string serverKey, string? database, int globalValue).
• AlertEngine gains IAlertThresholdOverrides? overrides = null as its last constructor parameter. Every existing call site (Lite MainWindow, DarlingWorker, every test) compiles unchanged, so no Lite or Darling file changed.
• Null gives exactly today's behaviour: Threshold() returns the global value without calling anything.
• The engine passes a database only for a ServerAndDatabase key. A database-level override can therefore never move a server-scoped check.
• If Resolve throws, the engine logs it and uses the global value. Otherwise one bad override would lose a whole check, or the whole sweep for a read outside a check's catch.

Part 3: every read site goes through Resolve. No _settings.<key member> read is left in the alerting project, and each key's resolution is pinned by a behaviour test (AlertThresholdKeyBehaviourTests).

Part 4: tests. See the test plan below.

The registry

Key	Scope
CpuThresholdPercent	Server
BlockingCountThreshold	Server
BlockingWaitSecondsThreshold	Server
DeadlockCountThreshold	Server
RunnableQueueWarningPerScheduler, RunnableQueueCriticalPerScheduler	Server
ClientTimeoutsWarningPerMinute, ClientTimeoutsCriticalPerMinute	Server
LongRunningQueryThresholdMinutes	Server
TempDbSpaceThresholdPercent	Server
LowDiskThresholdPercent, LowDiskThresholdGb	Server
DiskCriticalFreePercent, DiskCriticalFreeGb	Server
PvsThresholdPercent, PvsFloorGb	Server and Database
FileGrowthRiseMb, FileGrowthVolumePercent	Server and Database
FileGrowthLookbackMinutes	Server
LongRunningJobMultiplier	Server
FailedJobLookbackMinutes	Server

Why these four keys take database scope (the design call later slices build on). A database scope is given only where the engine compares the threshold against one database's own row:

• The version store (PVS) alert grades each ADR database's version store.
• The file growth alert grades each database file.

Every other check compares a figure for the whole server:

• a count across all databases (blocking, deadlocks);
• a server or volume reading (CPU, runnable queue, client timeouts, tempdb, volume free space);
• or a value the read itself filters on before any database is seen (the long-running query minutes, the job multiplier, and both lookback windows). A per-database value for one of these would need a read change.

DatabaseScope_IsExactlyTheChecksThatGradeEachDatabase pins this list.

Not tunable, each listed in the same file with its reason:

• CooldownMinutes: a cooldown. It spaces repeat notifications and decides no fire.
• PoisonWaitThresholdMs: retired as a threshold by Alert-condition semantics campaign: window-invariance, persistence gates, engine parity (8 findings + parity table) #3539. The engine no longer reads it.
• LongRunningQueryMaxResults: a read cap. It bounds how many sessions the card lists and decides no fire.
• SelfDiskFreeWarnPercent, SelfDiskFreeWarnGb, CollectionStaleMinutes, CollectionFailureThreshold: self-monitoring (self_alerts.*), about the monitor itself, not a monitored server.
• All 16 enable flags (AlertsEnabled, CpuEnabled, ... ForcePlanFailureEnabled).
• The non-integer members: five long-running-query filter flags, two exclusion lists, ExcludedDatabases, DeadlockRateThresholds (tiers that grade a fire, not decide one), and CpuAlertMode.
• Route keys are not on this contract: alert routing lives in each host's own settings.

Engine sites changed (AlertEngine.cs)

• CPU: the breach test in CheckCpuAsync, and the threshold text, numeric threshold and short message in FireCpuAsync.
• Blocking count: the RollingCountAlertGate bar and the fire's threshold text and number. Blocking wait: thresholdSeconds.
• Deadlocks: the excluded-database recount guard, the gate bar, and the fire's threshold text and number.
• Runnable queue and client timeouts: both bars of each pair. Each bar resolves on its own, and the evaluators already clamp the pair back into order on every evaluation (ClampBars), so a crossed override cannot invert them.
• Long-running query: the minutes passed to the read, and the fire's threshold text and number.
• tempdb: the breach test and the fire's threshold text and number.
• Volume free space: the breach bars, the critical tier bars, and the fire's threshold text and number.
• PVS:
  • The server-level percent gates the read: 0 turns the check off for that server, as the global 0 did.
  • Each database is then held to its own percent and floor through a new GetBreachedPvsDatabases overload that takes a per-database lookup. A database whose percent resolves to 0 never breaches.
  • The card states the pair the worst database was held to.
• File growth:
  • The lookback (server scope) goes to the read.
  • Each file is held to its own database's rise and level bars, through a new GetBreachedFiles overload. The Database File Growth's rise arm re-fires every cooldown against the same hourly observation: up to twelve cards per growth event — #3579's mechanism, one condition over #3636 level-gate observation guard uses the same per-database bar.
  • The threshold line states the worst file's database bars.
• Long-running job: the multiplier passed to the read and shown on the card.
• Failed jobs: the lookback passed to the fetcher and shown on the card.

The two old AlertContextBuilders signatures delegate to the new overloads, so Lite's callers and tests are unchanged.

Pins updated deliberately

• AlertReadFailureSurfaceTests: AlertEngine.cs exempt count 6 to 7, total exempt 41 to 42, plus an exemption entry. The new catch is the threshold-override fallback: an in-memory lookup, not a read. The check it serves still reads and counts exactly as before.
• FileGrowthRiseUnitCensusTests: the threshold line now names the resolved local {fileGrowthLookback} instead of {_settings.FileGrowthLookbackMinutes}. The unit and window wording it pins is unchanged.

Test plan

• dotnet build of PerformanceMonitor.Alerting, Lite/PerformanceMonitorLite.csproj, Lite.Tests and Darling.Tests (which builds the Darling service): Build succeeded, 0 warnings.
• New AlertThresholdOverrideTests (8):
  • null overrides give the global bars, and an empty overrides object gives identical outcomes;
  • an override on server A moves A (blocking count, and the long-running query minutes that travel to A's read) and leaves B;
  • a lowered PVS bar fires A only, and the card states 50;
  • a database override on A's shop removes shop from A's card while B's shop still fires;
  • a file growth database override holds each file to its own database, and the lookback override reaches the read;
  • server-scoped keys are never handed a database, and a database row for one moves nothing;
  • a crossed paired-bar override stays in order;
  • a throwing override falls back to the global value.
• Proved once by reverting: with Resolve ignored in the engine, 6 of the 7 behaviour tests then present failed. The throwing-override test passes either way, by design.
• AlertEngineTests unchanged and green. The harness only gained an Overrides property that defaults to null.
• Darling.Tests, one run: every *Census* and *Ratchet* class plus every alerting class (AlertEngine*, AlertThreshold*, AlertReadFailureSurfaceTests, DarlingAlertingTests, DarlingSelfAlertTests, PostgresAlertEvaluatorTests, RunnableQueue*, EscalationRestartSeedTests, AlertStatementCutOrderTests, AlertFiringLogTests, MuteRuleServerIdTests, AlertDelivererInventoryPinTests, AlertReadRetrySeamTests, BuiltinAlertPersistenceRungTests, NotificationRoutingTests, DarlingMcpAlertToolsTests, ChangelogIndexAndArchiveTests). Total 1485, failed 0, skipped 24 (live PostgreSQL classes, no rig in this lane).
• Lite.Tests: every *Census* and *Ratchet* class plus McpWritePolicyTests, PlanForceNoWritePathTests, FileGrowthAlertTests, AlertContextBuildersTests, AlertReadFailureSurfaceTests, LiteAlertForwardingTests, AzureMasterLiteWiringTests, ArchiveResetRestoresStateTests, CrossSkuSurfaceSourceTests, AlertFiringLogTests, AlertCadenceLiteTests. Total 220, failed 0.
• Full suites and the live PostgreSQL classes: not run on this laptop; CI decides.

Changelog

The changelog entry is reported to the coordinator's wave buffer and spliced after the wave lands; this PR does not touch CHANGELOG.md (the parallel-wave rule).

Leftovers

None in this slice. As the issue plans:

• Darling's store-backed override loader and its two PostgreSQL count thresholds are Per-server threshold overrides in Darling #5916.
• The override write path is a later slice.
• Lite gets no write path and no feature.

For the reviewer: an override's value is used as-is. The later loader has to apply each setting's own range validation (the Darling settings accessors clamp on read, for example DiskCriticalFreePercent to 0..100), and the IAlertThresholdOverrides doc says so.

Round 1 review

Claude posting for Erik Darling. Pushed in one commit (a6c711a); the PR's last round.

1. Blocking: the census breaks the Tests that assert spelling, not behavior: cap them, stop the review asking for pins, delete the shape ones #5833 ruling. AlertThresholdKeyCensusTests is deleted whole: the two reflection rosters, the enum roster, the exact-list pin of the database scope, and the source-text regex scan. The prose in AlertThresholdKey.cs no longer says a census test fails; it says the key list and NotTunable divide the settings by hand and that each key's resolution is pinned by a behaviour test.
2. The census's guarantee, as behaviour. New AlertThresholdKeyBehaviourTests (Darling/Darling.Tests/AlertThresholdKeyBehaviourTests.cs):
   • ServerOverride_MovesThatServersOutcome_ForEveryKey_AndLeavesTheOther, a [Theory] over every AlertThresholdKey value (21 cases). Each key has a fixture that breaches or grades at the global value; the baseline must be non-empty and the same on both servers, an override for server A must change A's outcome, and server B must be unchanged. The fixtures reuse the AlertEngineTests harness and builders. Outcomes compared: the metric, severity, current value and threshold line for most keys; the arguments the read was handed for LongRunningQueryThresholdMinutes and FileGrowthLookbackMinutes; the stated failure window for FailedJobLookbackMinutes. The two paired keys are graded by severity (a Critical override moves a Critical to a Warning).
   • DatabaseOverride_MovesThatDatabaseOnly_ForEveryDatabaseScopedKey, a [Theory] over the keys whose scope is ServerAndDatabase (PvsThresholdPercent, PvsFloorGb, FileGrowthRiseMb, FileGrowthVolumePercent): an override for database shop removes shop and not ledger from A's card, and shop on B still fires. The case list comes from the enum and ScopeOf, not from a hand-kept roster.
   • Every key has a fixture; none is left out. A key added later fails the theory with "has no fixture" until one is written.
3. The PVS gate. AlertThresholdDatabasePathTests: PvsServerPercentOfZero_TurnsTheCheckOffForTheServer_ADatabaseOverrideCannotTurnItOn and PvsDatabasePercentOfZero_NeverBreaches_WhileTheServersBarIsOn. This is the intended design: 0 on the server's own PVS percent is the server's switch for the check, and database overrides tune the bar, they do not turn the check on or off.
4. The Database File Growth's rise arm re-fires every cooldown against the same hourly observation: up to twelve cards per growth event — #3579's mechanism, one condition over #3636 re-fire guard on the per-database path. FileGrowth_DatabaseLevelOverride_SameHourlyObservationAcrossTwelveCooldowns_FiresOnce_ThenResolves (global level bar 1% would re-fire every cooldown; the database's 90% override makes the breach rise-only, so the same observation fires once across twelve cooldowns and resolves on the next collection), and its mirror FileGrowth_DatabaseLevelOverride_ThatMakesTheFileALevelBreach_StillRefiresEveryCooldown.
5. Non-blocking: the card's threshold line. It states the worst row's bars while other rows are held to other bars (AlertEngine.cs PVS and file growth cards). Accepted for this slice: no host supplies overrides yet. Per-server threshold overrides in Darling #5916 (the Darling loader) owns the card copy when per-database bars become real.

Proof the new tests catch the defect, by reverting in place and rerunning AlertThreshold*: reading _settings.FileGrowthVolumePercent in the level gate fails both new file growth tests; reading _settings.DiskCriticalFreeGb fails the DiskCriticalFreeGb case; dropping the pvsPercent <= 0 gate fails the PVS server-zero test. All three reverted; the tree is the committed one.

Run after the push: Darling.Tests AlertThreshold*, AlertEngine*, *FileGrowth*, AlertReadFailureSurfaceTests, *Attention*, *RunnableQueue*, every *Census* and *Ratchet*: 1101 total, 0 failed, 34 skipped (live PostgreSQL). Lite.Tests *AlertThreshold*, *AlertEngine*, *FileGrowth*, AlertReadFailureSurfaceTests, every *Census* and *Ratchet*: 66 total, 0 failed. PerformanceMonitor.Alerting, Lite, Lite.Tests and Darling.Tests all Build succeeded. The earlier "Darling.Tests 1485" figure above counted the deleted census class and an earlier test list; the full suite is left to CI.

[claude]

The #5833 ruling forbids this whole class. .github/REVIEW-TRAPS.md, Tests and censuses:

Ruling (#5833): tests are load-bearing. The censuses, pins and rosters named in this section are being deleted under #5833. Never add one, and never answer a trap with a test that reads source text or asserts how code is spelled (a signature at a line, a SQL fragment's wording, a comment's text, a trait roster, a coverage percentage). A trap is answered with a behaviour test that fails on the defect and passes on the fix, or with one sentence in the PR body saying why none is needed.

AlertThresholdKeyCensusTests is five of the exact shapes that sentence names:

• EveryIntegerMember_IsAKeyOrListedAsNotTunable and EveryMember_IsExactlyOneOfKeyOrNotTunable — a reflection roster over IAlertEngineSettings cross-checked against two hand-maintained lists.
• EveryKey_NamesAnIntegerMember_HasAScope_AndReadsThatMember — a roster over the enum plus Scopes.Count.
• DatabaseScope_IsExactlyTheChecksThatGradeEachDatabase — an exact-list pin of a design call.
• NoAlertingSource_ReadsAKeyMemberAroundTheOverrides — a regex scan of PerformanceMonitor.Alerting/*.cs, i.e. a test that reads source text and asserts how the code is spelled. This is the single shape the ruling calls out by name.

The design leans on the mechanism too: PerformanceMonitor.Alerting/AlertThresholdKey.cs:31 sells the registry on "A census test fails when a member is on neither list, so a threshold added later cannot reach the engine without a decision about whether it is tunable." Under #5833 that sentence has no enforcement behind it once the class goes, so the registry's stated safety property is load-bearing on a test that is not supposed to exist.

What the ruling asks for instead: the behaviour the registry buys is already pinned by AlertThresholdOverrideTests — ServerScopedKeys_AreNeverHandedADatabase is the behaviour test for the scope table, and the seven other facts cover resolution. Delete AlertThresholdKeyCensusTests, keep the two lists as documentation with their reasons (they read well as that), and put the "a new threshold must be added to one list" requirement in the file's own remarks plus one sentence in the PR body. A key added later that nobody routes through Threshold() is then caught the normal way: by the check's own behaviour test when an override fails to move it.

Note also that the source-text test is blind in the direction that matters, which is the second trap in the same section ("A roster-based census does not see the file it does not roster"): Directory.GetFiles(..., "*.cs") is non-recursive, the match is per line so a read wrapped across two lines is invisible, and the pattern only knows the settings. / _settings. spelling — a local alias (var s = _settings; … s.CpuThresholdPercent) reads clean. So it is both forbidden and not a guarantee.

[claude]

Non-blocking, but worth a sentence in the file: the PVS card lists up to five breached databases (BuildPvsPressureContext) under one threshold line, and that line now states only worst.DatabaseName's pair. With a per-database override live, the other listed rows were graded against a different bar than the one the card names — an operator reading "40% of database (floor 1 GB)" over a row that was actually held to 70% has no way to tell from the card.

This is the #3631 shape from .github/REVIEW-TRAPS.md ("A threshold that means different units at different surfaces … State the unit at every surface that shows the number") one step over: not the unit, the bar. Same thing at :3269 for file growth, where the card lists every breached file.

Unreachable in this slice (no host wires IAlertThresholdOverrides yet), so it is not a defect today — but it will be the moment #5916 lands the loader, and it is cheaper to decide now. Two options that both close it: put each row's own bar in the detail row the context builder renders, or keep the one line and say it is the worst row's. The code comments here and at :3269 record the choice but not the hazard.

[claude]

Non-blocking: the asymmetry here is documented, but it is one no test pins and the one an override author will get wrong. The server-level resolve of PvsThresholdPercent is a kill switch — resolve it to 0 for a server and CheckPvsPressureAsync returns before the read, so a database row on that server that overrides the percent up to 50 is never consulted. Every other key's database row is additive on top of the server row; this one can be vetoed by it.

FileGrowth has no equivalent (:3177 gates only on _settings.FileGrowthEnabled, which is not a key), so PVS is alone in the registry in having a server-scoped reading of a ServerAndDatabase key act as a gate.

AlertThresholdOverrideTests covers server-override-lowers (ServerOverride_ThatLowersTheBar_…) and database-override-raises (DatabaseOverride_AppliesToThatDatabaseOnly), but not server-0-with-a-database-row. A behaviour test for that one fact — "a server-level 0 silences the check even where a database overrides it up" — is what keeps #5916's loader from quietly treating 0 as "unset" and resurrecting the check.

[claude]

Verdict: CHANGES REQUESTED

Findings are inline: the #5833 ruling on AlertThresholdOverrideTests.cs:268, plus two non-blocking notes on AlertEngine.cs:3045 and :3087.

Claims

• VERIFIED — "overrides = null is the last constructor parameter; every existing call site compiles unchanged, so no Lite or Darling file changed" — AlertEngine.cs:398; the diff touches nothing under Lite/ or the Darling service, and Lite/MainWindow.xaml.cs:347 / DarlingWorker.cs:7591 still end at readFailures:.
• VERIFIED — "No host supplies overrides yet" — grep for IAlertThresholdOverrides outside the Alerting project hits only AlertEngineTests.cs:463 and the new test file.
• VERIFIED — "Null gives exactly today's behaviour, without calling anything" — AlertEngine.cs:437-443.
• VERIFIED — "A database is passed only for a ServerAndDatabase key" — AlertEngine.cs:445, pinned by ServerScopedKeys_AreNeverHandedADatabase.
• VERIFIED — "A throwing Resolve is logged and the global value applies" — AlertEngine.cs:450-456, pinned by AThrowingOverride_FallsBackToTheGlobalValue.
• VERIFIED — "No _settings.<key member> read is left in the alerting project" — a grep of all 21 member names returns nothing, and all 21 keys appear in a Threshold(AlertThresholdKey.…) call in AlertEngine.cs (463, 464, 952, 1219, 1441, 1695, 1872, 2244, 2425, 2593, 2797, 2915-2918, 3047, 3059, 3088, 3089, 3182, 3223, 3352, 3466).
• VERIFIED — "the evaluators already clamp the pair back into order (ClampBars)" — RunnableQueueEvaluator.cs:129-133 applied at :250; AttentionEvaluator.cs:158-159 applied at :336.
• VERIFIED — "the old AlertContextBuilders signatures delegate, so Lite's callers and tests are unchanged" — AlertContextBuilders.cs:395-397 and :1234-1235; Lite.Tests/FileGrowthAlertTests.cs still calls the four-argument form at 16 untouched sites. No doc break either: neither project sets GenerateDocumentationFile, so the now-ambiguous crefs raise no CS0419.
• VERIFIED — "the #3636 level-gate guard uses the same per-database bar" (AlertEngine.cs:3223), and "DatabaseScope_IsExactlyTheChecksThatGradeEachDatabase pins this list" (AlertThresholdOverrideTests.cs:327-342, exactly the four bolded keys).
• VERIFIED — the counts: 8 and 5 [Fact]s in the two new classes; AlertEngineTests' only diff is the Overrides property plus overrides: Overrides on Build(); and IAlertEngineSettings declares 54 properties (21 bool, 28 int, 5 other) of which 21 are keys and 33 are on NotTunable, so the two lists partition the contract.
• UNVERIFIED — "dotnet build … Build succeeded, 0 warnings" — this sandbox refuses dotnet build. Checked by hand instead: the (int,int)→(double,double) conversion at AlertEngine.cs:3059, the 3-arg/4-arg overload arities, h.Now being an instance property (AlertEngineTests.cs:454), RepoFile.PathTo existing (RepoFile.cs:57).
• UNVERIFIED — "Darling.Tests 1485/0/24" and "Lite.Tests 220/0" — Darling.Tests targets net10.0-windows; this is a Linux checkout with no rig.
• UNVERIFIED — "Proved once by reverting: 6 of the 7 behaviour tests then present failed" — a measurement on an earlier state of the branch. The tree now holds 8, of which 7 read Calls or an override's effect, so the figure is consistent with one test added after the experiment but cannot be re-run here.

Riskiest lines

1. Darling/Darling.Tests/AlertThresholdOverrideTests.cs:349 — NoAlertingSource_ReadsAKeyMemberAroundTheOverrides, a regex scan of PerformanceMonitor.Alerting/*.cs: the exact shape the #5833 ruling names, and the mechanism AlertThresholdKey.cs:31 sells the registry on — UNPINNED (it is the pin, and it is the finding).
2. PerformanceMonitor.Alerting/AlertEngine.cs:3047 — the resolved server-level PvsThresholdPercent gates the read, making it a kill switch a database row cannot override up; the only key whose server reading vetoes its database reading — UNPINNED (the two existing tests cover the other directions).
3. PerformanceMonitor.Alerting/AlertEngine.cs:3223 — anyNewObservation's level gate, now per-database; the wrong database here re-fires a rise-only breach every cooldown, the #3636 defect this guard exists for — pinned on the null path by FileGrowth_TheLevelArm_StillRefiresEveryCooldown_OnTheSameObservation and FileGrowth_SameHourlyObservationAcrossTwelveCooldowns_FiresOnce_ThenResolvesOnTheNextCollection; UNPINNED on the per-database path (DatabaseOverride_OnFileGrowth_… sweeps once).

Traps near this diff

• "Ruling (#5833): tests are load-bearing. The censuses, pins and rosters named in this section are being deleted under #5833. Never add one, and never answer a trap with a test that reads source text or asserts how code is spelled (a signature at a line, a SQL fragment's wording, a comment's text, a trait roster, a coverage percentage)." — applies — AlertThresholdOverrideTests.cs:268 adds AlertThresholdKeyCensusTests: four rosters plus a source-text regex at :349-358, with AlertThresholdKey.cs:31 making the registry's stated guarantee depend on it.
• "A roster-based census does not see the file it does not roster (#3699). A wider roster or a class-sweeping census is not the answer." — applies — AlertThresholdOverrideTests.cs:353 uses a non-recursive Directory.GetFiles, matches line by line, and knows only the settings./_settings. spelling, so a wrapped read or a local alias is invisible.
• "A threshold that means different units at different surfaces … State the unit at every surface that shows the number (#3631)." — applies, non-blocking and unreachable in this slice — AlertEngine.cs:3087-3092 and :3269-3274 state only the worst row's bars on cards that list other rows held to other bars.
• "An alert evaluated every ~30 s against an observation that changes once per COLLECTION re-fires on every cooldown expiry against the same rows … remember the observation stamp it last fired on, per key." — does not apply — the #3636 guard is intact and its level arm uses the same bar the breached set was built from, AlertEngine.cs:3223 against :3193.
• "An alert that fires on ONE breaching sample and resolves on the next (#3692). Presence-shaped conditions sit behind AlertPersistenceGate at K consecutive collections." — does not apply — only the compared value moved; AlertEngine.cs:966, :2826 and :1788 still call ObservePersistenceAsync with the same sample counts.
• "A default interface member compiles a parity gap away (#3640) … its members are required." — does not apply — IAlertThresholdOverrides.cs:36 declares Resolve with no default body; the optionality is on the engine's constructor parameter.
• "Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit." — does not apply yet — the change is wholly in the shared project and both hosts pass nothing (Lite/MainWindow.xaml.cs:347, DarlingWorker.cs:7591). Noted only because AlertEngine.cs:382 pre-commits the divergence in prose; #5916 is where it bites.

Divergence (blind read vs PR body)

The body presents AlertThresholdKeyCensusTests as the slice's strength — "every read site goes through Resolve … and a census test holds that" — and AlertThresholdKey.cs:31 builds the registry's safety property on it. .github/REVIEW-TRAPS.md rules out that exact mechanism by name under a ruling number, and the body never mentions the ruling or argues an exception. Once the class goes, "a threshold added later cannot reach the engine without a decision about whether it is tunable" is unbacked prose.

Two smaller gaps the same way, both inline and non-blocking: the body states the worst-row threshold line as finished design without noting the same card lists rows graded against other bars; and its PVS bullet is accurate but does not say this makes PVS the only key whose server row can veto a database row, which no test pins.

Everything else matched the blind read, including both deliberately-updated pins, and the "For the reviewer" note already owns the range-validation question I would otherwise have raised.

Reviewer checklist

• Build PerformanceMonitor.Alerting, Lite/PerformanceMonitorLite.csproj, Lite.Tests and Darling.Tests. This tree could not compile, and the diff turns two builder methods into overload pairs.
• Run both suites on Windows and confirm 1485/0/24 and 220/0, in particular AlertReadFailureSurfaceTests (18/7 and 42) and FileGrowthRiseUnitCensusTests.
• Settle the #5833 question before #5916 lands: drop the census class and keep the lists as prose, or record an explicit carve-out in .github/REVIEW-TRAPS.md.
• With a stub override in a scratch host, page one real PVS and one real file-growth card breaching on two databases with different bars, and read whether the single threshold line is defensible to an on-call operator.

[claude]

Follow-up review of a6c711a20 ("Replace the key census with behaviour tests; pin the PVS gate and the per-database re-fire guard"), the only commit since the prior full review (dba9b76d3).

This commit is the fix for that review's CHANGES REQUESTED. All three riskiest-lines findings from the prior round are addressed:

• The Tests that assert spelling, not behavior: cap them, stop the review asking for pins, delete the shape ones #5833 ruling violation — AlertThresholdKeyCensusTests (two reflection rosters, the enum roster, the database-scope exact-list pin, and the NoAlertingSource_ReadsAKeyMemberAroundTheOverrides source-text regex scan) is deleted whole from AlertThresholdOverrideTests.cs. The doc comment on AlertThresholdKey.cs:31-33 no longer claims a census test backs the registry; it now says the two lists divide the settings by hand with no enforcement, and that each key's resolution is pinned by a behaviour test instead.
• The PVS kill-switch, previously UNPINNED — new AlertThresholdDatabasePathTests.PvsServerPercentOfZero_TurnsTheCheckOffForTheServer_ADatabaseOverrideCannotTurnItOn pins that a server-level PvsThresholdPercent of 0 turns the check off and a database override cannot turn it back on. Matches AlertEngine.cs:3045-3047 (pvsPercent <= 0 gate before any per-database read).
• The Database File Growth's rise arm re-fires every cooldown against the same hourly observation: up to twelve cards per growth event — #3579's mechanism, one condition over #3636 per-database re-fire guard, previously UNPINNED — new AlertThresholdDatabasePathTests.FileGrowth_DatabaseLevelOverride_SameHourlyObservationAcrossTwelveCooldowns_FiresOnce_ThenResolves and its mirror ..._ThatMakesTheFileALevelBreach_StillRefiresEveryCooldown exercise the level gate at AlertEngine.cs:3218-3223 (BreachesLevelGate(f, Threshold(AlertThresholdKey.FileGrowthVolumePercent, key, f.DatabaseName))) through a per-database override, on both sides (rise-only re-fires once; level-breach still re-fires every cooldown).

New AlertThresholdKeyBehaviourTests.ServerOverride_MovesThatServersOutcome_ForEveryKey_AndLeavesTheOther is a [Theory] over Enum.GetValues<AlertThresholdKey>() (21 cases, matches the enum), and DatabaseOverride_MovesThatDatabaseOnly_ForEveryDatabaseScopedKey sources its case list from AlertThresholdKeys.ScopeOf, not a hand-kept roster — both self-updating, and the Fixture/DatabaseFixture switch's NotSupportedException default arm makes a future key without a fixture fail loudly rather than silently pass. This is the shape the ruling asks for: a behaviour test that fails on the defect and passes on the fix, not a test that reads source text or asserts how code is spelled.

The helper visibility bumps (private → internal on GrowingFile, HourlyCollection0/1, RiseOnlyFile, LevelOnlyFile in AlertEngineTests.cs; Runs, Window, Count, StormWindow in AttentionAlertTests.cs) are consumed only within Darling.Tests via using static Darling.Tests.AlertEngineTests; and a qualified AttentionAlertTests.StormWindow(...) call — no InternalsVisibleTo needed, no cross-assembly exposure.

Could not run dotnet build/dotnet test in this sandbox (the harness requires approval for them here), so the build/test-count claims in the PR body are unverified from this read-only pass; the reviewer checklist below covers that.

[claude]

Verdict: LGTM

Claims

• VERIFIED — "AlertThresholdKeyCensusTests is deleted whole: the two reflection rosters, the enum roster, the exact-list pin of the database scope, and the source-text regex scan" — confirmed in the diff of Darling/Darling.Tests/AlertThresholdOverrideTests.cs (the whole class, including NoAlertingSource_ReadsAKeyMemberAroundTheOverrides, is removed).
• VERIFIED — "The prose in AlertThresholdKey.cs no longer says a census test fails; it says the key list and NotTunable divide the settings by hand and each key's resolution is pinned by a behaviour test" — AlertThresholdKey.cs:31-33.
• VERIFIED — "ServerOverride_MovesThatServersOutcome_ForEveryKey_AndLeavesTheOther, a [Theory] over every AlertThresholdKey value (21 cases)" — AlertThresholdKeyBehaviourTests.cs:212,217-235; the enum (AlertThresholdKey.cs:37-57) has exactly 21 members.
• VERIFIED — "The case list comes from the enum and ScopeOf, not from a hand-kept roster" — AlertThresholdKeyBehaviourTests.cs:214-215 (AllKeys/DatabaseScopedKeys call Enum.GetValues and AlertThresholdKeys.ScopeOf).
• VERIFIED — "Every key has a fixture; none is left out. A key added later fails the theory with 'has no fixture' until one is written" — Fixture's and DatabaseFixture's switch default arms throw NotSupportedException with that exact phrase (AlertThresholdKeyBehaviourTests.cs:164,264).
• VERIFIED — PVS gate tests (PvsServerPercentOfZero_TurnsTheCheckOffForTheServer_ADatabaseOverrideCannotTurnItOn, PvsDatabasePercentOfZero_NeverBreaches_WhileTheServersBarIsOn) match the production gate — AlertEngine.cs:3045-3047.
• VERIFIED — the #3636 per-database re-fire guard tests match the production level gate — AlertEngine.cs:3218-3223 (BreachesLevelGate(f, Threshold(AlertThresholdKey.FileGrowthVolumePercent, key, f.DatabaseName))).
• VERIFIED — "No host supplies overrides yet" (repeated from the base feature, unchanged by this commit) — IAlertThresholdOverrides still appears only in PerformanceMonitor.Alerting and the Darling test files.
• UNVERIFIED — "Proof the new tests catch the defect, by reverting in place and rerunning AlertThreshold*" (three named reverts, each failing a specific new test) — a live dotnet test run; this sandbox's dotnet build/dotnet test require approval and could not be executed here.
• UNVERIFIED — "Run after the push: Darling.Tests ... 1101 total, 0 failed, 34 skipped ... Lite.Tests ... 66 total, 0 failed" and "PerformanceMonitor.Alerting, Lite, Lite.Tests and Darling.Tests all Build succeeded" — same reason; no build/test rig available in this read-only pass.

Riskiest lines

1. Darling/Darling.Tests/AlertThresholdDatabasePathTests.cs:325 — pins the server-level PVS kill switch (pvsPercent <= 0) that the prior full review marked UNPINNED — pinned by PvsServerPercentOfZero_TurnsTheCheckOffForTheServer_ADatabaseOverrideCannotTurnItOn (itself, new in this commit).
2. Darling/Darling.Tests/AlertThresholdDatabasePathTests.cs:361 — pins the #3636 re-fire guard on the per-database path, UNPINNED in the prior round — pinned by FileGrowth_DatabaseLevelOverride_SameHourlyObservationAcrossTwelveCooldowns_FiresOnce_ThenResolves (itself, new in this commit).
3. Darling/Darling.Tests/AlertThresholdKeyBehaviourTests.cs:164 — the NotSupportedException default arm standing in for the deleted reflection completeness census — pinned by ServerOverride_MovesThatServersOutcome_ForEveryKey_AndLeavesTheOther's [Theory] over all 21 enum values (itself; a 22nd key with no case throws here).

Traps near this diff

• "Ruling (#5833): tests are load-bearing. The censuses, pins and rosters named in this section are being deleted under #5833. Never add one, and never answer a trap with a test that reads source text or asserts how code is spelled... A trap is answered with a behaviour test that fails on the defect and passes on the fix, or with one sentence in the PR body saying why none is needed." — applies, and this commit IS the fix for the prior round's finding on this exact trap: the census class is gone (AlertThresholdOverrideTests.cs, old :268-364) and replaced by AlertThresholdKeyBehaviourTests.cs and AlertThresholdDatabasePathTests.cs, both behaviour tests that fail on the defect (confirmed by the prior reviewer's revert-and-rerun, itself UNVERIFIED here but structurally consistent with the code).
• "A roster-based census does not see the file it does not roster... A wider roster or a class-sweeping census is not the answer: each tool gets a behaviour test that fails when its page is cut without truncated." — does not apply anymore — the one roster/regex census this trap's shape matched in this area (NoAlertingSource_ReadsAKeyMemberAroundTheOverrides) is deleted outright in this commit, not widened.
• "InternalsVisibleTo on PerformanceMonitor.Analysis names Lite.Tests only — a helper Darling.Tests must call has to be public (#3688)." — does not apply — the private→internal bumps (GrowingFile, HourlyCollection0/1, RiseOnlyFile, LevelOnlyFile in AlertEngineTests.cs; Runs, Window, Count, StormWindow in AttentionAlertTests.cs) are consumed only inside the same Darling.Tests assembly (AlertThresholdKeyBehaviourTests.cs:15 using static, and a qualified AttentionAlertTests.StormWindow call at :181), so internal is sufficient and no InternalsVisibleTo is involved.

Divergence (blind read vs PR body)

None — the body's "Round 1 review" section's account of this commit (what was deleted, what replaced it, which prior findings it fixes) matches the diff on every claim graded above.

Reviewer checklist

• Build PerformanceMonitor.Alerting, Lite/PerformanceMonitorLite.csproj, Lite.Tests and Darling.Tests, and run the full Darling.Tests/Lite.Tests suites to confirm the body's reported counts (1101/0/34 and 66/0) and the three named revert-and-rerun failures — none of this could be executed in this read-only, approval-gated sandbox.

Claude posting for Erik Darling. Round 1's findings were fixed in the next push and round 2 answered LGTM on the current head; this stale changes-requested verdict is dismissed so the branch rule reads the current one.