Wire the engine-capability question into every read that can answer it (#2532)

[erikdarlingdata]

The user-visible half of #2532. The other half — the PostgreSQL AppliesTo axis and the IL guard it needed first — is #2536, also against dev. Neither depends on the other; see Merge order at the bottom.

What was wrong

#2511 wired the capability helper into the reads whose collectors are gated off on Azure SQL Database — twelve collectors' worth. That was the right scope for the edition axis, because a collector that runs on every SQL Server had nothing to say.

The engine-kind axis changes that arithmetic completely: on a PostgreSQL target every SQL Server read is a permanent gap, not just twelve. So get_database_config, get_wait_stats, get_top_queries_by_cpu and the rest still answered

unavailable — No database configuration data available. The config collector may not have run yet.

…about a collector that does not run on that engine at all. True, and pointing at the wrong cause. EngineCapabilityMissLivePostgresTests.APostgresTarget_AnswersNotCollectedNamingTheEngine_WhileAnUnprobedServerKeepsItsMiss asserts that behaviour explicitly today, which is how the gap stayed visible rather than becoming folklore.

Once #2530's UI waves land a human pointed at a PostgreSQL target sees PostgreSQL tabs and never asks a SQL Server read. But an agent over MCP has no tabs — it asks by name — and agents are half the product's surface.

What it does now

not_collected — aurora-01 runs Aurora PostgreSQL. The wait_stats collector is written against SQL Server and the dispatch gate's engine half never sends it at another engine, so this server does not collect the sys.dm_os_wait_stats cumulative totals, and never will. This is a permanent engine capability gap, not a collection outage: checking collection health, enabling a collector or starting a capture cannot change it.

72 Darling reads and 71 Lite reads ask the question, up from 17 on each. Every call site sits after the read came back empty — never before it — so a server whose registry row says one engine while its collected rows say another still gets its DATA rather than a confident explanation of why it cannot have any. That is the property DarlingEngineCapability's "called only on the miss path" note exists to protect, and it is why #2532's Option 3 (answer at the resolver) was not taken: it is cheaper and it is the one shape that can hand a mis-registered server an explanation instead of its rows.

CapturePathByCollector gains 28 noun phrases, so the sentence names a surface — "the sys.dm_os_wait_stats cumulative totals", "the sys.dm_io_virtual_file_stats per-file I/O totals" — instead of falling back to "the data this read is served from".

What I deliberately did NOT wire, and why

#2532 warned against blanket-wiring 83 reads without thinking, and four groups genuinely have a better answer than not_collected or no answer to give:

Not wired	Why
analyze_server, get_analysis_facts, compare_analysis, get_daily_summary, get_daily_summary_range, get_server_summary	each rolls up a dozen collectors and can name none of them. Naming one would be arbitrary and the message would be specific about the wrong thing. The honest answer is a rollup-level one — "every input to this read is a SQL Server collector" — which needs a second message shape this PR does not build. This is the residue, and it is the one worth filing if it is not closed here.
get_collection_health, get_collection_log	served by collection_log, which a PostgreSQL target genuinely populates. Their existing messages are about collection, and on an Aurora target they are correct. Wiring these would have been the same mistake in the other direction.
list_servers, get_fleet_overview, get_store_metrics, add_servers, remove_server, the six alert tools, the eight custom-view tools	resolve no single server, so there is no engine to speak for. get_alert_history takes an optional server and is fleet-wide by default; get_ag_health already handles that shape correctly and is the model if these ever need it.
analyze_plan_xml	parses XML the caller supplies. No store read, no server.
get_wait_trend / get_perfmon_trend's "not a wait type / counter this server collects" branches	only reached when the collector HAS produced rows, so the engine cannot be in question there. Their sibling unavailable branches — "nothing collected at all" — are wired.
the eight get_pg_* reads	wired in #2536, where the axis they need lives.

get_blocked_process_reports (Lite-only) and get_blocking (Darling-only) are each wired on their own SKU; they are not compared by the parity pin because the pin only looks at tools both surfaces expose.

One signature change

Lite's analyze_query_store_plan fetches the plan live from the monitored instance rather than from the store, so it was the one plan tool with no LocalDataService to ask. Its three siblings all take one. I added it — DI-injected, no user-facing parameter — because the capability question is the same question and BothSkus_WireTheSameReadsToTheSameCollectors is right to insist the two SKUs answer it identically. On Lite it is inert today (engineKind is still passed as null, which I re-checked rather than assumed), and it cannot become wrong.

Two pins changed meaning, both still derived

• EveryWiredRead_NamesARealCollectorThatIsGatedOffOnAzureSqlDb → …NamesARealCollectorWhoseCapabilityBranchCanFire. This is weaker and the doc comment says so. The Azure-specific claim was right while the edition axis was the only one; the kind axis makes every SQL Server collector a permanent gap on a PostgreSQL target, so demanding an Azure gap would refuse the wiring this issue is about. What survives is the property that matters at a call site — the branch is reachable on some (kind, edition) pair the store can hold — and it is derived by asking NotCollectedMessage, not by a list.
• EveryCapturePathEntry_NamesARealCollectorThatIsActuallyGatedSomewhere now requires an entry to be gated by its own AppliesTo somewhere or named by a wired read. Two derived arms, no hand list, and it catches dead prose in both directions: an entry for a collector nothing gates and nothing reads now fails.

AllNineHealthParserReads_AskTheQuestion and BothSkus_WireTheSameReadsToTheSameCollectors are untouched and both still hold.

Verification

The suites are net10.0-windows and cannot run on macOS, so CI is the arbiter — but the wiring is a source-scan property, so all of it was run locally in a throwaway net10.0 harness against the shipped MCP trees and the real built PerformanceMonitor.Collectors, using the same regexes the pins use:

• 72 Darling / 71 Lite wired reads found — the scan parsed something, which is the failure mode a source guard has;
• every wired collector name resolves in CollectorCatalog, has a CapturePathByCollector entry, and has a reachable capability branch;
• zero drift across the 55 tools both SKUs expose;
• both health-parser families still nine;
• every capture-path entry is gated-by-its-own-gate or wired;
• and the answers that must NOT change did not: wait_stats on a SQL Server box is still silent, and so is any collector when the kind is unknown (a pre-PostgreSQL targets have no UI on either SKU, and both UIs render SQL Server tabs at them #2530 row, or Lite).

Proven red: dropping one collector's CapturePathByCollector entry fails the wiring pin naming the tool and the collector; pointing a wired read at a name the catalog does not carry fails the first assertion. Both were run.

Both SKUs and both test projects build clean on macOS with EnableWindowsTargeting.

Merge order

#2536 and this PR both edit CollectorEngineCapability.CapturePathByCollector (adjacent entries) and EngineCapabilityMissTests.cs. The shared hunks in the test file are byte-identical in the two branches on purpose — the wiring-pin rewrite and the capture-path rewrite are the same edit on both sides, so they merge without a conflict whichever lands first. The only region this PR touches alone is the live-Postgres test, which #2536 leaves untouched. Just the dictionary needs the second PR to carry both sets of entries, which is a one-hunk rebase. Neither PR depends on the other's behaviour.

The pin that caught the one real defect

EngineCapabilityMissLivePostgresTests deliberately pinned the LIMIT this PR removes — that get_database_config still answered unavailable on a PostgreSQL target — so the gap could not later be mistaken for a regression. It is the one check that went red here, which is the pin doing exactly what it was written for. It now asserts the closed shape, plus get_wait_stats beside it: a read from a family with nothing to do with configuration, whose collector is gated off on no engine edition at all, so nothing but the kind axis could ever make it speak.

Closes nothing on its own. #2530 stays open for the UI waves; #2532 stays open for the rollup residue named above.

🤖 Generated with Claude Code

[claude]

Review

Reviewed this against CONTRIBUTING.md (no T-SQL touched here — this is a pure C# MCP-wiring change, so the T-SQL style rules don't apply) with a focus on correctness, Lite/Darling parity, security, and performance.

Verification performed:

• Cross-checked all 28 new CapturePathByCollector noun-phrase keys against the actual .Name properties on the corresponding PerformanceMonitor.Collectors classes (CpuSchedulerStatsCollector, DatabaseSizeStatsCollector, IndexObjectStatsCollector, MemoryGrantsCollector, PvsStatsCollector, QuerySnapshotsCollector, PlanCorrectionCollector, SessionStatsCollector, WaitingTasksCollector, ProcedureStatsCollector, RunningJobsCollector, PerfmonStatsCollector, LongQueryCompletionsCollector, DefaultTraceEventsCollector, QueryStoreHealthCollector, DatabaseScopedConfigCollector, TraceFlagsCollector, DatabaseConfigCollector, ServerConfigCollector, ServerPropertiesCollector, and the rest) — no typos, no dictionary duplicates.
• Confirmed the newly-wired collectors (cpu_utilization, wait_stats, query_stats, etc.) really do have AppliesTo(target) => true / run on every SQL Server edition, so the "permanent PostgreSQL-target gap" premise for the KIND axis holds and these entries won't collide with the EDITION-axis semantics EveryCapturePathEntry_NamesARealCollectorThatIsActuallyGatedSomewhere relies on.
• Independently recounted the wired call sites by grepping both DarlingEngineCapability.NotCollectedStatusAsync(postgres, ...) and McpEngineCapability.NotCollectedStatusAsync(dataService, ...) call sites (including the two multiline ones in DarlingMcpAgTools.cs/DarlingMcpHealthParserTools.cs and Lite's McpHealthParserTools.cs) — got 72 Darling / 71 Lite, matching the PR description and CHANGELOG exactly.
• Spot-checked ordering in multi-tool files (Lite/Mcp/McpQueryTools.cs, Darling's DarlingMcpQueryStoreRegressionTools.cs/DarlingMcpQueryHeatmapTools.cs) where a private EmptyAsync/EmptyRegressionsAsync/EmptyHeatmapAsync helper calls the capability check — the pin test's "last McpServerTool mark before the call" heuristic depends on file order, and in every file I checked the helper sits between its own tool's attribute and the next tool's, so attribution is correct.
• AnalyzeQueryStorePlan's new LocalDataService dataService parameter (Lite) is DI-injected (no [Description], not a user-facing MCP parameter) and mirrors its three sibling plan tools — consistent with how the framework already distinguishes injected services from tool arguments elsewhere in this file.
• No SQL built via string concatenation of user input; ServerEngineSql/GetSqlEngineEditionAsync reads are parameterized and unchanged by this PR.

Parity: Every collector name used on the Darling side has a byte-identical counterpart on the Lite side for the same tool (verified via the full McpServerTool(Name = ...) tool inventories and the NotCollectedStatusAsync call sites in both trees). get_blocking (Darling-only) and get_blocked_process_reports (Lite-only) are each wired only on their own SKU, matching the PR description's note that the parity pin only compares tools both surfaces expose.

Correctness: The miss-path-only placement (if (rows.Count == 0) return ... NotCollectedStatusAsync(...) ?? ...) is preserved everywhere — no call site risks returning not_collected for a server that actually has data. NotCollectedStatusAsync swallows its own read failures and returns null, so a broken registry lookup falls through to the existing unavailable/empty message rather than surfacing a new failure mode.

Nothing correctness-, security-, or parity-affecting found. One truly minor, non-blocking observation: in a handful of trend handlers (e.g. GetBlockingTrend/GetDeadlockTrend in both DarlingMcpBlockingTools.cs and Lite/Mcp/McpBlockingTools.cs), the capability check is placed after the capture-count probe query rather than before it, so a gated engine pays for one extra query on the miss path that other trend handlers (e.g. GetMemoryTrend) avoid by checking first. Since this only fires on an already-empty/rare path, it's not worth blocking on.

[claude]

Minor efficiency nit, not specific to this call site — it's the same shape in ~8 trend tools in both Darling and Lite (GetDeadlockTrend, GetWaitTrend/get_wait_trend's miss branch, GetMemoryTrend, GetFileIoTrend, GetQueryDurationTrend, GetProcedureDurationTrend, GetQueryStoreDurationTrend, and their Lite twins).

captures/points is fetched before the new NotCollectedStatusAsync gate check, but is only used in the EmptyTrend(...) call that's skipped when gated != null. For an engine with a permanent capability gap (e.g. any SQL-Server-only trend tool called against a PostgreSQL target), this branch is hit on every single call, forever — not a one-off miss — so the wasted capture-count query repeats indefinitely for that server/tool combination rather than being a rare-path cost.

Would it make sense to run the gate check first and only fetch captures when it comes back null? That would turn the permanently-gated case into one query instead of two, without changing behavior for the ungated case (SQL Server targets, which is the common case today).

[claude]

Review

Reviewed the diff against CONTRIBUTING.md (C# style, Lite/Darling parity) with a focus on correctness, parity drift, security, and performance. This is C#-only — no T-SQL changed, so the T-SQL style rules don't apply here.

Parity check (mechanical, not just eyeballed): extracted every NotCollectedStatusAsync(...) call added in this diff from both SKUs and diffed the collector-name arguments. Darling and Lite add the exact same 28 distinct collector names, with identical per-name call counts (54 new call sites each). The two SKU-exclusive tools (get_blocking on Darling, get_blocked_process_reports on Lite) both wire to "blocked_process_report" and land at the same count, so nothing drifted between the two apps.

Dictionary/call-site correctness: the 28 new CapturePathByCollector entries are exactly the 28 distinct collector-name strings used at the new call sites (no orphaned entries, no typo'd names that would silently fall back to the generic phrasing). Spot-checked several against CollectorCatalog.cs (CpuUtilizationCollector, QuerySnapshotsCollector, IndexObjectStatsCollector, PlanCorrectionCollector, PvsStatsCollector, etc.) — all resolve to real collectors.

Signature change: Lite's AnalyzeQueryStorePlan gains a LocalDataService dataService parameter, placed first — matching the exact position used by its three sibling methods (AnalyzeQueryPlan, AnalyzeProcedurePlan), so the DI-injection convention is preserved. Confirmed it's otherwise unused (engineKind still resolves to null on Lite, as the PR description states), so no behavior change there today.

One minor finding, posted inline on DarlingMcpBlockingTools.cs: in the trend tools (~8 in each SKU), the pre-existing captures/points query runs before the new capability gate check, even though it's discarded when the gate fires. For a server with a permanent capability gap (any SQL-Server-only trend tool against a PostgreSQL target) that's not a rare miss-path cost — it repeats on every call. Low severity, easy to defer.

Nothing else stood out — no SQL injection surface (no new dynamic SQL), no secrets, and the miss-vocabulary instructions text update is byte-identical between DarlingMcpInstructions.cs and McpInstructions.cs as required by the parity pin test.

[erikdarlingdata]

Right, and the part of that worth recording is the cost argument rather than the query count: this is not a rare-path waste that amortises. On an engine with a permanent gap the branch is taken on every call, forever, so the extra query repeats indefinitely for that server/tool pair — and the probe could never have told a gated engine anything, because there is nothing for it to find.

Fixed in 151434a across six reads on both SKUs:

• get_blocking_trend / get_deadlock_trend — the collection_log capture-count denominator, which is only consumed by EmptyTrend(...).
• get_wait_trend / get_perfmon_trend — the distinct-values probe. Those needed a slightly bigger edit than a reorder: the gate had been folded into the ?? on the collected.Count == 0 branch, so it is now a hoisted gated block above the probe and the fallback goes back to its original single form.

Both queries are still on the miss path, so the ordering does not touch the property that matters — the DATA read has already come back empty before either runs. The ungated case (every SQL Server target, which is all of them today) is byte-identical.

Checked the rest of the list and the other five are already in the right order: get_memory_trend, get_file_io_trend, get_query_duration_trend, get_procedure_duration_trend and get_query_store_duration_trend pass their existence probe to EmptyTrendAsync as an argument, and the gated block sits above that whole statement — so the probe task is never created when the gate fires. Same for get_current_waits_trend and get_blocking_stats on both SKUs.

[claude]

Reviewed against CONTRIBUTING.md conventions, Lite/Darling parity, correctness, security, and performance. This is a C#-only change (no T-SQL touched), so the T-SQL style rules don't apply here.

No issues found. Specifically checked:

• Collector-name correctness: traced all 55 newly-wired NotCollectedStatusAsync call sites against CollectorCatalog/PerformanceMonitor.Collectors/*.cs — every collector name passed (e.g. wait_stats, database_scoped_config, index_object_stats, memory_grant_stats, query_snapshots) is a real collector and matches what the read actually queries. No typos, no mismatched collector-to-read wiring.
• Lite/Darling parity: walked each touched file pair (DarlingMcp*Tools.cs vs Mcp*Tools.cs) and confirmed the same reads are wired to the same collector names on both SKUs. The one asymmetry (get_blocking Darling-only, get_blocked_process_reports Lite-only) is the documented, pre-existing SKU-exclusive pair and is correctly excluded from the parity pin. DarlingMcpInstructions.cs / Lite/Mcp/McpInstructions.cs got the identical added sentence, and McpMissMessageParityPinTests.cs now pins the new PostgreSQL clause — good.
• Shared logic untouched: CollectorEngineCapability.cs's decision functions (NotCollectedMessage, IsCollectedOnEngineKind, IsCollectedOnEngineEdition) are unchanged; this PR only adds CapturePathByCollector prose entries. So the risk surface really is just "did each call site name the right collector," which checks out.
• Ordering changes (engine check moved before the distinct-value probe in get_wait_trend/get_perfmon_trend/etc.): correctly preserves existing behavior for a healthy engine (gate returns null, falls through unchanged) while collapsing two queries into one for a permanently-gated engine.
• Security: all new queries are parameterized ($1 positional Npgsql / DuckDBParameter), and the collectorName arguments are all compile-time string literals, not user input. No injection surface.
• Test coverage: the updated EveryWiredRead_NamesARealCollectorWhoseCapabilityBranchCanFire and EveryCapturePathEntry_NamesARealCollectorThatIsActuallyGatedSomewhere derivations look sound and correctly weakened only where the PR body explains (kind axis makes every SQL Server collector a permanent PostgreSQL-side gap, so the old Azure-specific claim would have refused this wiring).

Nice work — this is a large, mechanically repetitive change and it's internally consistent throughout.

[claude]

Reviewed the diff (466/-155 across 41 files), the PR description, CollectorEngineCapability.cs, both *EngineCapability.cs helpers, and the source-scan pin tests (EngineCapabilityMissTests.cs / McpMissMessageParityPinTests.cs).

Lite/Darling parity — verified, not just trusted. I extracted every NotCollectedStatusAsync(...) call site from both Darling/PerformanceMonitor.Darling.Service/Mcp/*.cs and Lite/Mcp/*.cs and grouped by collector-name argument. The per-collector call counts match exactly between the two SKUs for all 33 keys touched by this PR (e.g. query_stats ×6/×6, wait_stats ×4/×4, blocked_process_report ×4/×4, deadlocks ×3/×3, index_object_stats ×3/×3, …). The 72-vs-71 tool count the PR description cites is fully explained by get_blocking (Darling-only) / get_blocked_process_reports (Lite-only), exactly as claimed — no drift found.

CollectorEngineCapability.CapturePathByCollector — confirmed no duplicate dictionary keys (would throw ArgumentException at static-init time) across the full file, and counted 28 new entries, matching the "28 noun phrases" claim in the PR body. Every collector-name string literal passed to NotCollectedStatusAsync across both SKUs matches a real Name => in a CollectorCatalog.All definition — no typos that would silently no-op the capability check (which is the specific failure mode EveryWiredRead_NamesARealCollectorWhoseCapabilityBranchCanFire exists to catch, and it will catch it via source scan since that test reads the files directly rather than a hand list).

Gate-before-probe reordering — spot-checked DarlingMcpTrendTools.GetPerfmonTrend and the blocking/deadlock/wait trend call sites in both SKUs: the new gated check is placed inside the existing if (....Count == 0) miss block, before the pre-existing secondary probe query, so it only ever fires on the already-empty path (matches the "called only on the miss path" invariant DarlingEngineCapability/McpEngineCapability's doc comments state) and avoids the extra query in the permanently-gated case, as intended.

AnalyzeQueryStorePlan's new LocalDataService dataService DI parameter (Lite) — confirmed LocalDataService is already registered via builder.Services.AddSingleton(_dataService) in McpHostService.cs, so the new constructor-injected parameter resolves correctly; placement and lack of [Description] matches the sibling tools' established DI-parameter convention.

No correctness, security, or performance issues found. This is a source-scan/derivation-tested change (the pin tests read the actual MCP source rather than a hand-maintained list), which is exactly the right shape for keeping 70+ call sites honest across two SKUs, and my independent extraction corroborates that it held here.