[erikdarlingdata]

Claude posting for Erik Darling.

Closes #6640. The base is not the default branch, so this will not auto-close it.

What changed

• PARAMETER_SENSITIVITY now sets ObjectName and Ranked. The read returns database_name, query_hash and query_plan_hash. The name is database/query_hash/query_plan_hash. The plan hash sits in the name because Figures holds numbers only. Value is the worker ratio. Figures carries min_worker_us, max_worker_us, grant_ratio and spill_divergence. Existing Metadata keys are unchanged.
• QUERY_SPILLS now sets ObjectName and Ranked. A second small statement on the same connection ranks (database, query_hash) by spilled pages, top three. It only runs when the aggregate found spills. The name is database/query_hash, and each ranked value is that query's own spilled pages. The fact's Value stays the window total, so ranked[0].value is at most value. This is the one stated exception to the RankedObject invariant. The aggregate figures and thresholds are untouched.
• The card text (Investigation) lists the named queries for both facts, so the finding text an agent reads names them.
• get_analysis_facts showed ranked only at two or more entries, and never showed object_name. A new FactRanked.AttachSubject adds object_name for these two keys only. Every other fact keeps its bytes. Both MCP tools (Darling and Lite) call it.
• Lite has the same change. No migration rung, facts are ephemeral.

Timing

Seed: one million query_stats rows in a 4 hour window, 20k distinct queries, about 14% with spills. The new statement was about 158 ms on Postgres 16 (cold 157, warm 159) and 3 to 5 ms on DuckDB. The existing aggregate was 285 to 317 ms on Postgres and about 4 ms on DuckDB, and it is unchanged. So the new read adds roughly half the aggregate cost and only when spills exist.

Measured on two production stores, on the server with the most spills in each over the last 4 hours. The new ranked read took 37.6 ms on the first run and 25.4 ms on the second on one store, and 36.8 ms and 27.0 ms on the other. The existing aggregate took 36.3 ms and 30.8 ms on the same servers. The new read runs only when the aggregate found spills.

Tests and revert proof

Test	Revert	Status
Darling FactsNameQueriesLiveTests (both)	Without the new columns and ranked list, ObjectName is null and Ranked empty, so the name asserts fail	Logic ran locally against Postgres 16 in a scratch harness (both passed). The xunit class itself waits for CI.
Darling FactsNameQueriesAdviceTests (3)	Without the text and AttachSubject, the sentence and object_name are missing	Logic ran locally in a scratch harness. The xunit class waits for CI.
Lite FactsNameQueriesTests (2)	Same as the Darling live tests	Builds locally. Lite.Tests needs the Windows desktop runtime, so it cannot run on macOS. Waits for CI.

The literal revert runs in CI on #6643, which is a throwaway PR. It stops the Darling and Lite collectors naming the queries, stops the card text naming them, and stops the payload adding object_name. Each test above should fail there. The results go here when that run ends. The full suites also wait for CI.

CHANGELOG entry

• PARAMETER_SENSITIVITY and QUERY_SPILLS now name their queries (database, query hash and, for parameter sensitivity, plan hash), worst first and at most three, in the finding text and in get_analysis_facts (object_name, ranked). Darling and Lite. (Analysis: PARAMETER_SENSITIVITY and QUERY_SPILLS facts name the queries behind them #6640)

Considered, not added

• A spill-ordered top-queries read: the issue puts it out of scope.
• object_name on facts other than these two: it would change other payload bytes.
• A separate plan_hash field: RankedObject has no slot for a string, so the plan hash is in the name.

CI fix (run 39981348396)

Root cause (Lite, 12 failures). QuerySpillsRankedSql selects SUM(v.delta_spills). DuckDB types SUM(BIGINT) as HUGEINT, and DuckDB.NET returns that as System.Numerics.BigInteger. Convert.ToDouble throws InvalidCastException on a BigInteger. CollectQueryStatsFactsAsync caught the exception, reported a collection failure and returned. So every QUERY_STATS fact was lost: QUERY_SPILLS, and QUERY_HIGH_DOP, which is built from the same read. The reader-close and second-command pattern was fine.

Fix. The ranked read now uses ToInt64(...) (the BigInteger-aware helper the aggregate already uses). The aggregate figures and thresholds are unchanged. The Darling side was never affected: Npgsql returns SUM(bigint) as numeric/decimal, which Convert.ToDouble handles.

Reproduction. I ran a throwaway net10.0 console on macOS against DuckDB.NET 1.5.5 with the same view shape and parameter style. The ranked query itself ran fine, and the value came back as BigInteger. Convert.ToDouble threw the exact InvalidCastException. ToInt64 returned the right sum. The console was deleted and none of it is committed.

Darling FactRankedTests.BothToolFiles_.... It pins the literal call shape FactRanked.Attach(new. The tool files now call FactRanked.Attach(FactRanked.AttachSubject(new { ... }, f, opts), f.Ranked, opts). AttachSubject returns a JsonObject, so Attach still appends ranked last. I updated the test's pattern and closing-tail assertion to that shape, with a comment. I kept the tool files as they are, so the get_analysis_facts bytes for other facts stay identical.

Other pins checked. The other source-text tests that read the touched files (ParameterSensitivityFiringSignatureParityTests, CreationTimeClockFrameDisciplineTests, QueryHighDopStaleMaxDopParityTests, QueryStoreSliceTieBreakSourceTests, ServerClockDstReadTests, McpToolAuditFactAdviceTextTests) do not match the new SQL or code. Darling.Tests and Lite.Tests build to 0 errors. They cannot run on macOS, so CI is the arbiter.

Review round (review 5630610823)

• Ranked invariant broken by QUERY_SPILLS: real. I kept the total as Value (the scorer thresholds on it) and made the exception explicit. The RankedObject doc and both get_analysis_facts descriptions now say so. The Lite test pins ranked[0].value < value. Needs Erik only if he would rather rank on a different quantity.
• Source pin FactRankedTests.BothToolFiles_...: real, already fixed in a5c9b5d (pattern updated to the AttachSubject shape). The new FactPayload_AttachSubjectThenAttach_... test checks the tool wiring by output, not by spelling: object_name and ranked both ride, ranked last.
• object_name undocumented: real. Both get_analysis_facts descriptions now state it for the two facts. The analyze_server descriptions name the two new root_fact.ranked cards. The Fact.Ranked doc no longer says no SQL Server collector sets it.
• FactIdentity roster: real. QUERY_SPILLS and PARAMETER_SENSITIVITY joined SqlServerKeys with Describe arms ("most spilled query ...", "most sensitive plan ..."), so a consumed hop names its query. The Lite roster test is updated.
• Non-blocking tiebreak on the parameter sensitivity read: real, small. Both SKUs now end ORDER BY worker_ratio DESC, database_name, query_hash, query_plan_hash. New Lite test ParameterSensitivity_TiedRatios_AreNamedInKeyOrder. Revert proof: drop the tiebreak and the tied plans come back in insert order, so the test fails (waits for CI, Lite.Tests is Windows-only).
• Non-blocking field placement and public constant: real, cosmetic. Fixed on both SKUs.
• root_fact carries no object_name on analyze_server: not added. The card's Investigation and the new Behind it: sentence name the query, and get_analysis_facts carries object_name. Adding it to root_fact changes every finding's bytes, which is a design choice for Erik. Recommendation: add it later if agents miss the name.
• Unverified claims (live Postgres test, Lite tests, Revert proof for #6641 (do not merge) #6643 revert): unchanged, they wait for CI. The two production timings in this body were measured on the stores named there.

Considered, not added

• A FactRankedTests.AssertInvariant call on the two new facts: the QUERY_SPILLS exception would fail it by design.
• A re-time with thousands of distinct queries: the read is capped by LIMIT 3 after a group by. The production timings above are the measure I have.

Root cards gain the query name (review of afe2d7d)

Both query facts are hops in today's graph (MEMORY_GRANT_PENDING, RESOURCE_SEMAPHORE, TEMPDB_USAGE and PARAMETER_SENSITIVITY reach QUERY_SPILLS, CPU_SPIKE and MEMORY_GRANT_PENDING reach PARAMETER_SENSITIVITY). Rostering them in FactIdentity means the root card of those stories now carries a "Behind it:" sentence naming the query, and that text is frozen into StoryText. That is what #6640 asks for, so it stays. The roster comment now says so.

[claude]

QUERY_SPILLS breaks the Ranked invariant: entry [0]'s value is not the fact's value.

RankedObject's doc states the invariant the whole seam rests on (PerformanceMonitor.Analysis/AnalysisModels.cs:11-14):

The invariant. Entry [0] IS the fact's own subject — Fact.ObjectName and Fact.Value — so a card has ONE truth about what it is about and the advice never has to decide between two spellings of the worst object.

FactRankedTests.AssertInvariant enforces exactly that, by value (Darling/Darling.Tests/FactRankedTests.cs:51-52):

Assert.Equal(fact.ObjectName, fact.Ranked[0].ObjectName);
Assert.Equal(fact.Value, fact.Ranked[0].Value, precision: 9);

Here Value = totalSpills (the window's total across every spilling query) while spillRanked[0].Value is the top query's own spilled pages. The name half of the invariant holds (line 132); the value half does not. Darling has the same shape at Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.QueryPerf.cs:133-136, and the PR's own tests pin the disagreement rather than catching it — Lite.Tests/FactsNameQueriesTests.cs:471 asserts total_spills == 1100 and :477 asserts ranked values 500, 300, 200. Dropping FactRankedTests.AssertInvariant(fact) into either new test fails today.

This is not only a doc mismatch: both SKUs' get_analysis_facts descriptions promise it to agents, verbatim —

entry [0] being the fact's own subject (same object_name and value)

Failure scenario: a window with 1,100 spilled pages across four queries emits value: 1100 on the card and ranked[0].value: 500. An agent (or analyze_server's root_fact, where the payload is { key, value } + ranked and there is no object_name at all — Lite/Mcp/McpAnalysisTools.cs:217-220) reads the two as the same quantity and reports 1,100 pages against Db/0xSB, or concludes the ranked list disagrees with its own card. PARAMETER_SENSITIVITY is fine here precisely because its Value is the worst offender's ratio.

Either keep entry [0] faithful to Value (e.g. rank on a quantity the fact's own Value states, and carry per-query pages in Figures), or make the exception explicit — amend RankedObject's invariant, FactRankedTests.AssertInvariant, and the sentence in both tool descriptions — rather than leaving three places asserting something the shipped fact no longer does.

[claude]

This wiring red-lines an existing test on both SKUs.

FactRankedTests.BothToolFiles_AttachTheRankedBlockOncePerTool_AndNeverSpellItAsAProperty reads both tool files as source and pins the two literals this hunk rewrote (Darling/Darling.Tests/FactRankedTests.cs:152-153):

Assert.Equal(1, CountOf(facts, "FactRanked.Attach(new"));
Assert.Contains("}, f.Ranked, McpHelpers.JsonOptions))", facts, StringComparison.Ordinal);

After the change the body reads FactRanked.Attach(FactRanked.AttachSubject(new (so "FactRanked.Attach(new" counts 0, not 1) and closes with }, f, McpHelpers.JsonOptions), f.Ranked, McpHelpers.JsonOptions)) (so the second needle, which requires } immediately before , f.Ranked, is absent). The loop runs over both SKUs, so both assertions fail twice — once for Lite/Mcp/McpAnalysisTools.cs:363,390 and once for Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpTools.cs:397,424. .github/ci-quarantine.txt is empty, so nothing exempts it.

It is a source-reading pin, so per the #5833 ruling it is not coverage and should not be answered with another one — but it is in the tree and it will fail the build. It needs updating (or deleting under that ruling) in this PR, and the PR body's "The full suites also wait for CI" does not mention it.

[claude]

A new payload field no tool description mentions — and the field's own doc now says the opposite of what the code does.

object_name reaches the wire here, but neither SKU's get_analysis_facts description was touched. The rule this repo already wrote for this exact seam (Darling/Darling.Tests/FactRankedTests.cs:135-137):

Both tools' descriptions state the field and its two-or-more rule, because a payload field an agent is not told about is a field it will not read.

The single-query case is the whole reason AttachSubject exists (ranked rides only at 2+), and that is precisely the case an agent will not look for, because Lite/Mcp/McpAnalysisTools.cs:278 and Darling/.../DarlingMcpTools.cs:312 still describe the fact entry as source / key / value / base_severity / severity / metadata / amplifiers plus ranked.

Two more stale statements in the same seam that this diff falsifies:

1. PerformanceMonitor.Analysis/AnalysisModels.cs:71-73 — Fact.Ranked's doc: "Empty for the overwhelming majority of facts … and no SQL Server collector sets this at all." Both new call sites are SQL Server collectors.
2. analyze_server's description enumerates who ranks — "A finding whose root fact ranked several objects (the autovacuum-disabled and vacuum-backlog cards, the bloat and growth trends) carries root_fact.ranked" — and QUERY_SPILLS / PARAMETER_SENSITIVITY now do too, on both SKUs, via RootFactRanked = rootFact?.Ranked ?? [] (InferenceEngine.cs:383).

Worth deciding at the same time whether analyze_server's root_fact should carry the subject too: it is { key, value } + attached ranked (Lite/Mcp/McpAnalysisTools.cs:217-220), so on the one-spilling-query finding — the case this PR is about — the query is named only in the English advice sentence, which is the thing ranked was introduced to stop agents from parsing.

[claude]

These two keys now name something, so they owe FactIdentity a row — otherwise the name vanishes whenever the fact is consumed as a hop.

FactIdentity is the roster of keys that name a thing, and its doc states the membership rule (PerformanceMonitor.Analysis/FactIdentity.cs:43-45): "membership is an explicit, pinned list per engine, and a key joins it by a one-row edit here that says what it names." SqlServerKeys (:76-82) is still BLOCKING_CHAIN, RUNNING_JOBS, ANOMALY_OBJECT_GROWTH, ANOMALY_OBJECT_CONTENTION.

Both new keys are edge destinations, so the greedy traversal consumes them under higher-severity roots routinely:

• RESOURCE_SEMAPHORE → QUERY_SPILLS (RelationshipGraph.cs:198)
• MEMORY_GRANT_PENDING → QUERY_SPILLS (:208)
• TEMPDB_USAGE → QUERY_SPILLS (:403)
• CPU_SPIKE → PARAMETER_SENSITIVITY (:168), MEMORY_GRANT_PENDING → PARAMETER_SENSITIVITY (:223)

Failure scenario: RESOURCE_SEMAPHORE at 1.25 consumes QUERY_SPILLS at 0.9. The only card the operator and the e-mail get is the root's, FactIdentity.Describe returns null for QUERY_SPILLS, so no Behind it: sentence is composed — and the spilling query this PR just taught the collector to identify is named nowhere on the surface that is actually read. That is the #3691 defect shape FactIdentity exists to close, now reachable on two more keys; before this PR there was no name to lose.

Two rows ("QUERY_SPILLS", "PARAMETER_SENSITIVITY") plus their Describe arms — e.g. query hash `…` the way SqlServerBadActorPrefix already renders one (:140) — would close it. If it is deliberately out of scope for #6640, a sentence in the PR body saying so would do.

[claude]

Non-blocking: the new field landed between CollectQueryStatsFactsAsync's doc comment (lines 16-19) and the method, so the field now carries two <summary> elements — "Collects query-level aggregate facts from query_stats. Focuses on spills…" plus the #6640 one — and the method is left undocumented. Darling got this right (PgFactCollector.QueryPerf.cs:65-86: the constant sits above the method's own summary). Moving the field above line 16 restores both.

Also minor parity: Darling declares public static readonly string QuerySpillsRankedSql while Lite's is private. Nothing references the Darling one (the new live test builds its facts through PgFactCollector), so private/internal on both would match.

[claude]

Non-blocking, but it is new as of this hunk: these three columns turn the parameter-sensitivity read's row order into published names, and that statement's ORDER BY worker_ratio DESC (line 269, and PgFactCollector.QueryPerf.cs:253) has no tiebreak. The new spills read deliberately added one — ORDER BY spills DESC, v.database_name, v.query_hash (line 34) — so the two halves of the same PR disagree.

Two plans at the same max/min worker ratio (10.0 is a common floor-hugging value, and the cap keeps only three) can swap places between passes, so the card's "Worst plans (database/query_hash/query_plan_hash): …" sentence and ranked[0] / object_name name a different plan each run, on identical data — and worstRatio, worstMinWorker etc. are read off whichever row landed first. Appending , database_name, query_hash, query_plan_hash to both SKUs' ORDER BY makes the pick stable and the two reads consistent with each other.

[claude]

Verdict: CHANGES REQUESTED

Collectors, advice sentence and payload helper do what the body says, and Lite/Darling behaviour parity is real. Not accounted for: QUERY_SPILLS now contradicts the Ranked invariant three places still assert, the tool wiring red-lines an existing pin on both SKUs, and object_name is documented nowhere an agent reads. Details in the six inline comments.

Claims

• VERIFIED — PARAMETER_SENSITIVITY sets ObjectName/Ranked; read returns database_name, query_hash, query_plan_hash; name is database/query_hash/query_plan_hash — DuckDbFactCollector.QueryPerf.cs:259-261, :317-321, :343-344; PgFactCollector.QueryPerf.cs:243-245, :308-312, :336-337.
• VERIFIED — Value is the worker ratio; Figures carries min_worker_us, max_worker_us, grant_ratio, spill_divergence — DuckDbFactCollector.QueryPerf.cs:323-330 (ordinal 2 is the column worstRatio reads at :311); Lite.Tests/FactsNameQueriesTests.cs:453-455.
• VERIFIED — existing Metadata keys unchanged — :345-354 and :134-140 untouched; every hunk additive.
• VERIFIED — second statement, same connection, ranks (database, query_hash) by spilled pages, top three, only when the aggregate found spills — DuckDbFactCollector.QueryPerf.cs:106-122, PgFactCollector.QueryPerf.cs:110-125, LIMIT FactRanked.MaxObjects (3, AnalysisModels.cs:44).
• VERIFIED — aggregate figures and thresholds untouched — QueryStatsSql unchanged; scorer arms FactScorer.cs:510, :515 read the same values.
• VERIFIED — card text lists the named queries for both facts — FactAdvice.cs:1640, :1695 via NamedQueriesSentence (:1658-1666); FactsNameQueriesAdviceTests asserts both wordings.
• VERIFIED — get_analysis_facts never showed object_name; AttachSubject adds it for these two keys only — FactRankedPayload.cs:77-95; old entry shape McpAnalysisTools.cs:365-389.
• VERIFIED — every other fact keeps its bytes — FactRankedPayload.cs:88-89 returns the caller's reference; FactsNameQueriesAdviceTests.cs:71-74.
• VERIFIED — both MCP tools call it — McpAnalysisTools.cs:363+:390, DarlingMcpTools.cs:397+:424.
• VERIFIED — Lite has the same change, no migration rung, facts ephemeral — Lite hunks mirror Darling's; no store/ladder/StorageVersion touched; AnalysisModels.cs:75-78.
• UNVERIFIED — 158 ms Postgres / 3-5 ms DuckDB vs 285-317 ms aggregate — live measurement over a 1M-row seed.
• UNVERIFIED — two production stores, 37.6/25.4 ms and 36.8/27.0 ms — those stores are unreachable here.
• UNVERIFIED — Darling live tests "passed in a scratch harness" — the class Assert.SkipWhens without DARLING_TEST_PG (FactsNameQueriesLiveTests.cs:170), so neither this tree nor CI runs it.
• UNVERIFIED — Lite tests "build locally" — Lite.Tests is Windows-desktop; no build here.
• UNVERIFIED — the revert PR #6643 fails each new test — needs that run.

No claim is refuted by the tree; the problems below are things the body does not claim either way.

Riskiest lines

1. Lite/Analysis/DuckDbFactCollector.QueryPerf.cs:133 (twin PgFactCollector.QueryPerf.cs:136) — Ranked[0].Value is one query's pages while Value is the window total, against AnalysisModels.cs:11-14, FactRankedTests.cs:52 and both tool descriptions ("same object_name and value") — UNCOVERED: AssertInvariant is never called on these facts and Lite.Tests/FactsNameQueriesTests.cs:471-477 pins the mismatch (1100 vs 500/300/200) as expected.
2. Lite/Mcp/McpAnalysisTools.cs:363 (twin DarlingMcpTools.cs:397) — the nested Attach(AttachSubject(...)) is the only thing putting object_name on the wire; a swapped nesting compiles and drops a block — UNCOVERED: the new test exercises AttachSubject directly, not the tool payload.
3. PgFactCollector.QueryPerf.cs:306-321 — ordinals 8/9/10 against the columns appended at :243-245, plus reader.CloseAsync() at :112 before a second command on one pooled connection — UNCOVERED in CI (live-only, skipped without DARLING_TEST_PG); the Lite twin covers Lite's ordinals and its close at :108.

Traps near this diff

• "Ruling (#5833): tests are load-bearing … never answer a trap with a test that reads source text or asserts how code is spelled" — applies: the tool wiring is governed only by a source pin (FactRankedTests.cs:152-153) that this change breaks on both SKUs; update or retire it, do not add another spelling pin.
• "Lite and Darling ship in parity: a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit." — does not apply to behaviour (every site changed on both SKUs: DuckDbFactCollector.QueryPerf.cs:22-35/:103-133/:255-261/:306-344 vs PgFactCollector.QueryPerf.cs:68-81/:107-136/:243-245/:306-337). It does bite the descriptions: neither SKU documents object_name. Cosmetic only: Lite's constant private vs Darling's public, and Lite's doc comment on the wrong member.
• "FactAdvice.PopulateStoryText runs BEFORE ClusterIntoIncidents/StampClusters/Reconcile, so a sentence added after folding must re-read the frozen StoryText blob (#3709)" — does not apply: appended inside ComposeQuerySpills/ComposeParameterSensitivity (FactAdvice.cs:1640, :1695), so it is in the blob from the start.
• "A count taken off the page and named as the population's … belongs on the statement above the LIMIT (#3679)" — does not apply: spilling_query_count is still COUNT(DISTINCT …) above any limit (:79-80, read at :138); the three names are a cap beside it, as is offender_count (:347).
• "ORDER BY 3 + 4 DESC is not an ordinal sum … sort by named columns (#3613)" — does not apply: the new read sorts named columns with a full tiebreak (:34, PgFactCollector.QueryPerf.cs:80). The adjacent hazard — ORDER BY worker_ratio DESC with no tiebreak now deciding published names — is inline on :261, non-blocking.
• "Every MCP filter is part of the query, not a post-filter on the page (#3641)" — does not apply: the ranked.Count < MaxObjects cut (:315) must follow the exact compiled-before-the-window test #4821 moved out of SQL (:294-299); the spills read, with no such test, does cut in SQL.
• "A fixture testing a shape no live run produces (#3709)" — does not apply: four sensitive plans and six spill snapshots in a four-hour window are real shapes, and both tests drive real collector code over planted rows.

Divergence (blind read vs PR body)

1. The body calls the QUERY_SPILLS entry's value "the spilled pages" without noting it is the first Ranked list whose entry [0] does not restate the fact's Value. RankedObject's invariant (AnalysisModels.cs:11-14), FactRankedTests.AssertInvariant (:51-52) and both tool descriptions still assert the old rule. PARAMETER_SENSITIVITY honours it; QUERY_SPILLS does not, and the PR's own tests pin the disagreement. Either the fact or the invariant has to move.
2. "Both MCP tools call it" is true, and it breaks FactRankedTests.BothToolFiles_AttachTheRankedBlockOncePerTool_AndNeverSpellItAsAProperty on both SKUs: CountOf(facts, "FactRanked.Attach(new") is 0 where 1 is expected, and "}, f.Ranked, McpHelpers.JsonOptions))" is gone. .github/ci-quarantine.txt is empty; "the full suites also wait for CI" is all the body says about existing tests.
3. No description or doc was updated for a field whose purpose is the single-query case: object_name is undocumented in both get_analysis_facts descriptions, Fact.Ranked's doc still reads "no SQL Server collector sets this at all" (AnalysisModels.cs:73), and analyze_server's description still names only the four PostgreSQL families, while these two ride there via InferenceEngine.cs:383. Related: root_fact is { key, value } + ranked (McpAnalysisTools.cs:217-220), so a one-spilling-query finding names the query only in prose.

Separately, FactIdentity's roster was not extended (inline on PgFactCollector.QueryPerf.cs:135): both keys are edge destinations (RelationshipGraph.cs:198, :208, :403, :168, :223), so a consumed hop's query name reaches no surface.

Reviewer checklist

• Build and run Darling.Tests.FactRankedTests to confirm the two source-pin assertions fail as read, and decide whether that pin is updated or retired under #5833.
• Run FactsNameQueriesLiveTests with DARLING_TEST_PG against a real Postgres 16 store: Darling's ranked read, its mid-method reader close plus second command on one pooled connection, and the 8/9/10 ordinals are otherwise unexercised.
• Build Lite.Tests on Windows and run FactsNameQueriesTests plus QuerySpillsDistinctQueryCountTests, which now also traverses the reader close.
• Read one real analyze_server and one get_analysis_facts payload for a QUERY_SPILLS finding and judge how value (window total) reads beside ranked[0].value (one query).
• Re-time the ranked read where a four-hour window spills across thousands of distinct (database, query_hash) pairs.

[claude]

Follow-up pass (base 23bacbd → head 398e3d4).

gh pr view 6641 --json commits shows this PR has exactly two commits: 23bacbd48 (already reviewed in full) and 398e3d486, a Merge remote-tracking branch 'origin/dev' into HEAD with no commits of its own in between. The only file difference in the follow-up range, Darling/tools/provision-roles.sql:3011-3015 (syncing the function-owner reassignment array with the p_actor-widened update_notification_route/set_notification_route_destinations signatures and the new set_notification_route_agent), is content the merge brought in from dev (commit 0a6121e1c, PR #6644) — unrelated to this PR's PARAMETER_SENSITIVITY/QUERY_SPILLS work. I checked it anyway: the three signatures in the array now match the CREATE OR REPLACE FUNCTION/REVOKE/GRANT EXECUTE statements for those functions exactly (same file, lines 1513, 1614, 1621, 1733, 1737, 1823–1827), so it isn't a defect even incidentally.

Per this pass's instructions, a merge's incoming content isn't the author's change unless a new commit interacts with it, and none does here. So this pass has nothing of this PR's own to review, and the verdict below reflects only that. It does not mean the prior full review's CHANGES_REQUESTED findings (the Ranked invariant mismatch on QUERY_SPILLS, the broken FactRankedTests source pin, undocumented object_name) are resolved — no commit in this range touches that code. Those still need a substantive follow-up commit before merge.

[claude]

Verdict: LGTM

Claims

• None in scope — the PR body was not revised in this range, and its claims about the PARAMETER_SENSITIVITY/QUERY_SPILLS work were already graded in the prior full-pass review (head=23bacbd482c863cace9cb4e532729a7bba402fc3). This pass's only commit range content (see Riskiest lines) carries no body claims of its own.

Riskiest lines

1. None in scope — the one file difference in this range, Darling/tools/provision-roles.sql:3011-3015, was brought in by 398e3d486's merge of origin/dev (commit 0a6121e1c, PR #6644), not authored by this PR, and no commit in this range interacts with it. Checked anyway: the three signatures (update_notification_route, set_notification_route_agent, set_notification_route_destinations) match the file's own CREATE OR REPLACE FUNCTION/REVOKE/GRANT EXECUTE statements exactly — not a defect.

Traps near this diff

• None evaluated — no author-authored change is in scope this pass (see Riskiest lines above).

Divergence (blind read vs PR body)

None — the PR body was not amended in this range, so there is nothing new to compare it against.

Reviewer checklist

• Confirm a future commit addresses the prior full review's unresolved CHANGES_REQUESTED findings (head=23bacbd482c863cace9cb4e532729a7bba402fc3): the Ranked invariant mismatch on QUERY_SPILLS, the broken FactRankedTests source-pin assertion on both SKUs, and the undocumented object_name field — this pass's only in-range content is an unrelated dev merge and does not touch them.

Claude posting for Erik Darling. Answered in 53778c2. The QUERY_SPILLS value exception is now stated in the invariant doc and tool descriptions. FactIdentity gained both keys. object_name is documented. Tiebreak and cosmetic fixes are in. Details are in the PR body under "Review round".

[claude]

The new head sentence puts get_analysis_facts over the 620-character served-head cap, on both SKUs.

The sentence added before <<GUIDE>> —

QUERY_SPILLS and PARAMETER_SENSITIVITY facts also carry object_name even for one query: the worst query as database/query_hash, or database/query_hash/query_plan_hash.

— is 168 characters. The head (everything before the marker, after TrimEnd) goes from 554 to 722 characters, and McpToolGuide.Served appends the 31-character GuidePointer, so the served head goes from 585 to 753.

Two pins cap that at 620, per SKU:

• Lite.Tests/McpToolGuideHeads.SqlCore.cs:89 — EveryConvertedHead_CarriesItsGuardrailFact_AndThePointer iterates ConvertedTools, which includes "get_analysis_facts" (line 31), and asserts served.Served.Length <= 620.
• Lite.Tests/McpToolGuideTests.cs:50 — EveryConvertedHead_StaysAtOrUnder620Characters, the roster-free backstop over every tool with a tail.

The Darling copy of the same description (Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpTools.cs:312) is byte-identical at 722 characters, so its twins fail too: Darling/Darling.Tests/McpToolGuideHeads.SqlCore.cs:90 and Darling/Darling.Tests/McpToolGuideTests.cs:50. Four failing tests, not one.

This family has no slack: analyze_server serves at 617 and get_analysis_findings at 610, both against the same 620 bar — which is why the PR body's "Other pins checked" census (ParameterSensitivityFiringSignatureParityTests, QueryHighDopStaleMaxDopParityTests, …) missing these two is the gap that matters. Darling.Tests and Lite.Tests building to 0 errors does not reach a length assertion.

The 1,000-character absolute cap in McpToolsListBudgetTests (ConvertedHeadCap) still passes at 753, so this fails only on the 620 target — the one the family is actually held to.

The field is worth documenting; the head is not where it fits. The <<GUIDE>> tail already carries the full statement ("entry [0] being the fact's own subject (same object_name, and the same value except on QUERY_SPILLS …)"), and the tail has ~21k characters of headroom against MaxGuideCharacters. Either move the object_name sentence into the tail, or compress it to a head-sized clause (e.g. object_name rides on QUERY_SPILLS and PARAMETER_SENSITIVITY even for one query. — 73 characters, which lands the served head at 658, still over; so it needs to come out of the head, or something else in the head has to go). Whichever way, re-measure both SKUs against 620 rather than against the 1,000 cap.

[claude]

Darling's half of the head-budget break (see the full accounting on Lite/Mcp/McpAnalysisTools.cs:278): this description is byte-identical to Lite's, so its head is also 722 characters and its served head 753, against the 620 target asserted at Darling/Darling.Tests/McpToolGuideHeads.SqlCore.cs:90 (get_analysis_facts is in that file's ConvertedTools at line 31) and at Darling/Darling.Tests/McpToolGuideTests.cs:50.

Parity is held correctly here — both SKUs carry the same sentence — so whatever shortens Lite's head has to shorten this one by the same bytes, or the byte-identity the family relies on breaks instead.

[claude]

Non-blocking, but this roster comment now misdescribes the two keys the same hunk added. The clause still reads "the others are rostered because they NAME something … not because an edge reaches them today" — and edges do reach both of them:

• RelationshipGraph.cs:198 RESOURCE_SEMAPHORE → QUERY_SPILLS, :208 MEMORY_GRANT_PENDING → QUERY_SPILLS, :403 TEMPDB_USAGE → QUERY_SPILLS
• RelationshipGraph.cs:168 CPU_SPIKE → PARAMETER_SENSITIVITY, :223 MEMORY_GRANT_PENDING → PARAMETER_SENSITIVITY

So the Behind it: sentence is live for these two, not rostered-but-dormant, which is the opposite of what the sentence tells the next reader. That matters more than usual here because this file's own summary calls a roster row "the reviewer's prompt to read that row", and .github/REVIEW-TRAPS.md has the inverse shape on record ("A key in the graph is not a key in the data; grep the collector that emits it", #3709) — a reader who trusts this clause would conclude the arms are inert and stop checking them.

Worth noting alongside it that FactScorer.cs:510 thresholds QUERY_SPILLS at 100/1000 pages, so a spilling window usually scores it at or above 0.5 and BuildStories takes it as its own root before a lower-severity root can consume it as a hop (the #3688/#3709 semantics). The hop sentence is therefore the uncommon path for this key and the card's own new Most spilled: sentence is the common one — both correct, but the comment reads as though only the hop path existed.

[claude]

Non-blocking: the QUERY_SPILLS value exception is stated in three of the four places that assert it, and the fourth is in this file.

Stated: AnalysisModels.cs:13 (the RankedObject invariant), and both SKUs' get_analysis_facts descriptions ("the same value except on QUERY_SPILLS, where it is that query's own spilled pages and the fact value is the window total").

Not stated: ToPayload's own doc, 30 lines above this at FactRankedPayload.cs:47 — "Entry [0] is the subject, and it is emitted. It repeats ObjectName / value that the enclosing payload already carries, on purpose … The invariant (RankedObject) is what makes the repetition safe — the two cannot disagree." For QUERY_SPILLS they now do disagree, and this is the paragraph a reader lands on when they are deciding whether ranked[0].value may be read as the card's own value. One clause pointing at the RankedObject exception closes it.

Same gap on the analyze_server side, where the body says it chose not to add object_name to root_fact: root_fact there is { key, value } plus ranked, so a QUERY_SPILLS finding emits root_fact.value = window total next to root_fact.ranked[0].value = one query's pages, and that description's ranked paragraph makes no value claim either way. Not wrong, just the one surface where an agent sees the two numbers side by side with nothing saying they are different quantities.

Also noting for the record that FactRankedTests.AssertInvariant (Darling.Tests/FactRankedTests.cs:52) still asserts fact.Value == fact.Ranked[0].Value. That is harmless today — its five callers are all PgTarget* families — and the PR body says so explicitly under "Considered, not added". Flagging only because the helper's own doc calls itself "the invariant, in one place", which is now one place out of two.

[claude]

Non-blocking, and an asymmetry with the Lite twin worth a sentence in the comment rather than a code change.

The aggregate and the ranked read are two statements on the same connection with the first reader closed between them, so on PostgreSQL they run in two implicit transactions. Lite's twin holds _duckDb.AcquireReadLock(...) for the whole method (Lite/Analysis/DuckDbFactCollector.QueryPerf.cs:41), so no collector write can land between its two reads; nothing here does the equivalent.

The window is [TimeRangeStart, TimeRangeEnd] and on an unanchored pass TimeRangeEnd is ~now, so a collection that commits between the two statements with collection_time <= TimeRangeEnd is admitted by the second read and not the first. If it carries spills for the current top query, ranked[0].Value can come back greater than Value — which is exactly the one thing AnalysisModels.cs:13 promises cannot happen ("never more than the total").

Narrow and self-healing (the next pass re-reads both), and the figures the card scores on are unaffected. But the invariant is now written down as an absolute, so either the window should be closed to the aggregate's own snapshot (a single REPEATABLE READ transaction around both reads, or reading the two from one statement) or the doc should say "never more than the total within one read". The comment above currently says "Same window, same filter" — true of the SQL, not quite true of the instant.

[claude]

Non-blocking: this reads the clock twice in one expression, and its own Lite twin reads it once.

DateTime.UtcNow.Ticks - (DateTime.UtcNow.Ticks % TimeSpan.TicksPerSecond)

The two UtcNow reads are independent, so if the second lands in the next second the subtraction is not a second-truncation at all (first read 10.999 s, second 11.001 s → 10.998 s). The Lite twin does it correctly off one read: Lite.Tests/FactsNameQueriesTests.cs:28-30, DateTime.UtcNow.Ticks / TimeSpan.TicksPerSecond * TimeSpan.TicksPerSecond.

It happens not to bite: every seed and both window bounds are derived from the single periodEnd value once it exists, and nothing asserts the truncation, so an untruncated anchor changes no assertion. Flagging it because it is .github/REVIEW-TRAPS.md's recorded shape verbatim — "A test that reads the clock twice measures the CI scheduler … One anchor owns both ends of an interval" (#3676) — and this is a live test whose future edits will inherit the anchor. Copying the Lite twin's expression costs nothing and takes the shape off the board.

[claude]

Verdict: CHANGES REQUESTED

Claims

• VERIFIED — PARAMETER_SENSITIVITY sets ObjectName/Ranked, named database/query_hash/query_plan_hash — PgFactCollector.QueryPerf.cs:243-245,:308-321; Lite :260-262,:318-331
• VERIFIED — Value is the worker ratio, Figures the four keys, Metadata unchanged — PgFactCollector.QueryPerf.cs:314-321 vs :338-347
• VERIFIED — a second statement ranks (database, query_hash) by spilled pages, top three, only when the aggregate found spills — Darling :68-81 under :110; Lite :18-31 under :107
• VERIFIED — QUERY_SPILLS keeps the window total as Value, the one stated exception — :133, AnalysisModels.cs:11-15, pinned Lite.Tests/FactsNameQueriesTests.cs:116
• VERIFIED — the card text names the queries for both facts — FactAdvice.cs:1640,:1726 via :1661-1666; asserted FactsNameQueriesAdviceTests.cs:60,:74
• VERIFIED — AttachSubject adds object_name for these two keys only, other facts keep their bytes, both tools call it — FactRankedPayload.cs:77-95; DarlingMcpTools.cs:429/438, McpAnalysisTools.cs:363/390; Assert.Same at FactsNameQueriesAdviceTests.cs:101-102
• VERIFIED — Lite has the same change; one cap, FactRanked.MaxObjects
• VERIFIED — no migration rung — no schema file in the diff; Ranked rides in memory only (InferenceEngine.cs:383)
• VERIFIED — the ranked read uses ToInt64 — Lite :120 → BlockingPairRowQuery.cs:96-98
• VERIFIED — both SKUs end ORDER BY worker_ratio DESC, database_name, query_hash, query_plan_hash — Darling :253, Lite :270; pinned by ParameterSensitivity_TiedRatios_AreNamedInKeyOrder
• VERIFIED — the two keys joined SqlServerKeys with Describe arms, Lite roster updated — FactIdentity.cs:83-84,:159-162; StoryNamedHopsTests.cs:36,:60-64; one reader outside the file (:121), both re-read
• VERIFIED — the Fact.Ranked doc no longer says no SQL Server collector sets it — AnalysisModels.cs:72-76
• VERIFIED — field placement and the non-public constant fixed on both SKUs — :68/:18
• VERIFIED — FactRankedTests.BothToolFiles_... updated to the new shape — :154-155 matches both tool files
• VERIFIED — root_fact carries no object_name — DarlingMcpTools.cs:251-254, McpAnalysisTools.cs:217-220
• VERIFIED but incomplete — "Other pins checked": the six named pins do hold (the firing-signature regex covers only the unchanged WHERE; the high-DOP guard is keyed on COUNT(... max_dop ...)). The census never reaches the head-budget pins this commit breaks — see REFUTED
• UNVERIFIED — the seeded timings (158 ms PG, 3-5 ms DuckDB, aggregate 285-317 ms) — needs the million-row rig
• UNVERIFIED — the two production timings — production reads
• UNVERIFIED — "Npgsql returns SUM(bigint) as numeric/decimal" — the code takes that path (:122) and the reasoning is sound, but the CLR type needs a live PostgreSQL; the only test skips without DARLING_TEST_PG (FactsNameQueriesLiveTests.cs:200)
• UNVERIFIED — the deleted scratch harnesses (PG 16 run, the DuckDB.NET BigInteger reproduction)
• UNVERIFIED — "#6643 … each test above should fail there" — another PR's run
• REFUTED — "build to 0 errors" as a sufficient check on the description edit: the 168-char sentence added before <<GUIDE>> takes the get_analysis_facts served head from 585 to 753 against a 620 cap asserted four times — Lite.Tests/McpToolGuideHeads.SqlCore.cs:89, Lite.Tests/McpToolGuideTests.cs:50, Darling.Tests/McpToolGuideHeads.SqlCore.cs:90, Darling.Tests/McpToolGuideTests.cs:50. A length assertion is not a compile error. Detail inline on McpAnalysisTools.cs:278
• REFUTED — the table's "(2)" for Lite FactsNameQueriesTests and "(3)" for FactsNameQueriesAdviceTests — the files carry 3 and 4 [Fact] methods; stale after the review round added two tests the later sections do name

Riskiest lines

1. Lite/Mcp/McpAnalysisTools.cs:278 (and byte-identical DarlingMcpTools.cs:312) — the only line breaking a shipped assertion, in a family with ~3 characters of slack (analyze_server 617, get_analysis_findings 610, cap 620) — covered by EveryConvertedHead_CarriesItsGuardrailFact_AndThePointer and EveryConvertedHead_StaysAtOrUnder620Characters, both SKUs; all four fail today
2. PgFactCollector.QueryPerf.cs:112 — closing the aggregate's reader mid-method and running a second command on the connection; a throw hits the catch at :164 and takes QUERY_HIGH_DOP down with QUERY_SPILLS, exactly the Lite BigInteger failure in run 39981348396 — UNCOVERED (FactsNameQueriesLiveTests skips without DARLING_TEST_PG)
3. Lite/Analysis/DuckDbFactCollector.QueryPerf.cs:344 / PgFactCollector.QueryPerf.cs:336 — ranked[0] indexed unguarded, safe only because every row incrementing offenderCount also appends while Count < 3 — covered on Lite by ParameterSensitivity_NamesTheOffenders_WorstFirst_WithinTheCap; UNCOVERED on Darling

Traps near this diff

• "Lite and Darling ship in parity … a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply — both collectors (:68/:18), both tool files and the shared halves move together; descriptions byte-identical at 722 chars
• "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — does not apply — no test pins the two QuerySpillsRankedSql equal, and the aggregate above is already per-SKU (v_server_config Lite :65 vs server_config Darling :40)
• "A count taken off the page and named as the population's (#3679)" — does not apply — spilling_query_count still comes from the un-limited COUNT(DISTINCT …) at :54-55
• "Truncation inferred from a full page … Fetch limit + 1" — does not apply — no truncated flag; the cap is a stated constant (AnalysisModels.cs:46)
• "ORDER BY 3 + 4 DESC … Sort by named columns." — does not apply — both statements sort by named columns (:80)
• "Integer division in a rate column … suspect until a 1.0 * or a CAST is found" — does not apply — the only division is the pre-existing cast-and-NULLIF'd ratio at :237; the new read only sums
• "CASE WHEN interval > 0 THEN delta / interval ELSE 0 END spells 'unknowable' as 0 … End at END (NULL)" — does not apply — no new rate or CASE; the 0.0 fallbacks sit on a sum whose row exists by construction (delta_spills > 0)
• "A test that reads the clock twice measures the CI scheduler … One anchor owns both ends of an interval (#3676)" — applies — FactsNameQueriesLiveTests.cs:101 reads UtcNow.Ticks twice where the Lite twin (FactsNameQueriesTests.cs:28-30) reads it once; no assertion depends on it, so non-blocking, noted inline
• "A NEW string constant in PgTargetFactKeys without a ByFactKey row (#3688)" — does not apply — no key added; both are pre-existing literals (FactScorer.cs:510,:515)
• "PopulateStoryText runs BEFORE ClusterIntoIncidents / StampClusters / Reconcile … a sentence added after folding must re-read the frozen StoryText blob (#3709)" — does not apply — appended inside the composers (FactAdvice.cs:1640,:1726)
• "A key in the graph is not a key in the data; grep the collector that emits it (#3709)" — does not apply as dormancy — both keys are emitted here and reached by live edges (RelationshipGraph.cs:198,:208,:403,:168,:223) — but FactIdentity.cs:72 now asserts the opposite, flagged inline
• "Never answer a trap with a test that reads source text or asserts how code is spelled … Never add one (#5833)" — does not apply — none added; FactRankedTests.cs:154-155 is an existing pin kept in step

Divergence (blind read vs PR body)

One divergence, and it is the finding. The body's "CI fix" and "Other pins checked" census covers the source-text pins that read the collector files, and it is right about every pin it names — but the commit's other half edits a tool description, and that surface has its own pinned budget the census never visits. My blind read measured the get_analysis_facts head at 722 characters against a 620-character served-head cap asserted four times across the two SKUs; "build to 0 errors" cannot see a length assertion. The body treats "object_name undocumented: real" as closed by adding a sentence to the head without stating what the head then measures.

Two smaller disagreements, both understating the body's own work: the table's "(2)" and "(3)" are now 3 and 4, and FactIdentity.cs:72 claims no edge reaches the newly rostered keys when five do. Everything else the body claims about the diff I confirmed from the tree, including the three things it declines to do.

Reviewer checklist

• Run Lite.Tests and Darling.Tests on Windows and confirm the four head-budget tests pass once the description is trimmed — the figures above are a character count, not a test run
• Run FactsNameQueriesLiveTests with DARLING_TEST_PG set: the only coverage for Darling's ranked read, the three-column projection off FROM latest, svr, and Convert.ToDouble over SUM(bigint)
• Call get_analysis_facts against a real spilling window: object_name on the two facts, ranked absent at one object, no other fact's bytes moved
• Confirm #6643's revert run actually fails each new test

[claude]

The roster doc is false for the two keys this PR adds, and that hides a behaviour change.

The sentence this hunk leads into says the keys other than BLOCKING_CHAIN / RUNNING_JOBS are rostered "because they NAME something … not because an edge reaches them today." Six edges in today's SQL Server graph reach the two keys this PR just added:

• RelationshipGraph.cs:198 RESOURCE_SEMAPHORE → QUERY_SPILLS
• RelationshipGraph.cs:208 MEMORY_GRANT_PENDING → QUERY_SPILLS
• RelationshipGraph.cs:403 TEMPDB_USAGE → QUERY_SPILLS
• RelationshipGraph.cs:440 PARAMETER_SENSITIVITY → QUERY_SPILLS
• RelationshipGraph.cs:168 CPU_SPIKE → PARAMETER_SENSITIVITY
• RelationshipGraph.cs:223 MEMORY_GRANT_PENDING → PARAMETER_SENSITIVITY

So the roster edit is not inert the way the comment implies. InferenceEngine.cs:356 calls FactIdentity.NamedHopsOf on every story path, and FactAdvice.cs:355 (WithNamedHops) appends one Behind it: … sentence per named hop to the root's investigation. From this PR forward, a story rooted at MEMORY_GRANT_PENDING, RESOURCE_SEMAPHORE, TEMPDB_USAGE or CPU_SPIKE that consumes either query fact below the 0.5 self-root floor gains a sentence it did not have — e.g. Behind it: `QUERY_SPILLS` (severity 0.40) — most spilled query `Sales/0xAB`: 1,100 pages (8.59 MB) spilled to tempdb — memory grants too small for their sorts/hashes. That text is composed by PopulateStoryText and frozen into the persisted finding's StoryText, so it reaches the viewer, the alert e-mail, analyze_server and get_analysis_findings on every existing install — not just the two cards this PR is about. MaxNamedHops is 3, so on a crowded path the two new names can also displace a BLOCKING_CHAIN or RUNNING_JOBS name that used to ride.

That may well be the behaviour you want, but the comment states the opposite of what the graph does, and neither the comment nor the PR body mentions the root-card text change. Please fix the sentence to say these two ARE reached as hops today and name the edges, and say in the body that root cards gain the sentence.

Nothing asserts the new sentence or its absence today, so this is also the one place a behaviour test would pay: a story rooted at MEMORY_GRANT_PENDING with a sub-0.5 QUERY_SPILLS on the path, asserting the root's investigation names the query.

[claude]

Non-blocking, but the doc on PgFactCollector.AllSql (PgFactCollector.cs:294-302) opens with "Every query this collector executes, for the ungated dialect/hygiene pins in Darling.Tests". QuerySpillsRankedSql is executed by this collector (line 113) and is not in that list, so that sentence is now false.

No live consequence here — I checked this statement against what those pins assert and it passes all of them: v_query_stats is a V4 passthrough view, parameters are $1–$3 positional, there is no QUALIFY, no bare NOW()/CURRENT_TIMESTAMP, no read_parquet, no any_value. The cost is forward-looking: the next edit to this statement is unguarded, and a reader who trusts the roster's own sentence will believe it is.

Note that registering it is not free — PgFactCollectorTests.AllSql_CoversEveryQuery_OnePerCollectMethodPlusTheDmvFallback pins AllSql.Count == LiteCollectMethodSurface.Length + 4, so adding the entry means moving that arithmetic and its comment too. Amending the AllSql sentence to say "one per collect method, plus the conditional second reads" would be the cheaper honest fix if you would rather not grow the roster.

[claude]

Non-blocking edge case, on both SKUs (same line at Lite/Analysis/DuckDbFactCollector.QueryPerf.cs:121).

A NULL database_name or query_hash folds to string.Empty and the name is still built as db + "/" + hash, so the operator-facing string can come out as `Sales/`, `/0xAB` or — when both are NULL — `/`. That string is not internal: it becomes Fact.ObjectName, which FactAdvice.NamedQueriesSentence prints verbatim ("Most spilled: Sales/ (500 pages (3.91 MB))") and FactIdentity.Describe wraps as "most spilled query /".

This is reachable rather than theoretical — the aggregate's own comment at line 50 says "A NULL hash collapses to one per database, as for spilling_queries", so the read is written in the knowledge that NULL hashes occur, and the COALESCE(v.database_name, '') at line 53 says the same for the database. Keeping the identity equal to the aggregate's is the right call; the question is only what the card prints. Falling back to a legible placeholder (or declining to name an entry whose hash is NULL, since an unnamed query is not a name an operator can act on) would keep the prose honest. Entirely your call — the counts are unaffected either way.

[claude]

Non-blocking, flagging the coverage shape rather than asking for a change.

Both tests in this class skip unless DARLING_TEST_PG is set, so on an ordinary CI run the Darling half of this PR — the three new columns in ParameterSensitivitySql, the new tie-break, the QuerySpillsRankedSql read, the ordinal reads at lines 308-310 and 120-122 of PgFactCollector.QueryPerf.cs — has no behaviour test that executes. Lite's FactsNameQueriesTests covers the twin logic, but not Darling's own SQL: the two statements differ (FROM latest, svr vs FROM latest, make_interval vs the de-skewed creation_time_utc, Convert.ToDouble vs ToInt64), and the ordinals are read positionally against each SELECT list separately. A wrong ordinal or a column dropped from the Postgres SELECT list would be green here and green in Lite.

There is also no Darling twin of Lite's ParameterSensitivity_TiedRatios_AreNamedInKeyOrder, even though the tie-break landed on both reads — so the Postgres ORDER BY worker_ratio DESC, database_name, query_hash, query_plan_hash is the one half of that fix with nothing exercising it.

If the live store is reachable in whatever runs these, a tie case here would close the gap; otherwise it is worth a line in the body saying the Darling collector half rests on the live run.

[claude]

Verdict: CHANGES REQUESTED

Claims

• VERIFIED — PARAMETER_SENSITIVITY sets ObjectName/Ranked, the read returns the three name columns, Value is the worker ratio, Figures carries the four numbers — PgFactCollector.QueryPerf.cs:243-245,306-337, DuckDbFactCollector.QueryPerf.cs:260-262,316-345; ranked[0] and worstRatio both come from the first kept row.
• VERIFIED — "Existing Metadata keys are unchanged" — both hunks insert above an untouched Metadata initializer (PgFactCollector.QueryPerf.cs:336-347).
• VERIFIED — the second statement runs on the same connection, top three, only when the aggregate found spills — PgFactCollector.QueryPerf.cs:110-125, DuckDbFactCollector.QueryPerf.cs:107-123.
• VERIFIED — "ranked[0].value is at most value" — the total sums every in-window row, the ranked read one group's delta_spills > 0 rows, and CollectorDeltaCalculator.cs:572-598 clamps a reset to 0 rather than a negative delta.
• VERIFIED — AttachSubject adds object_name for these two keys only, every other fact keeps its bytes, both tools call it — FactRankedPayload.cs:88-89, McpAnalysisTools.cs:363, DarlingMcpTools.cs:429; pinned by FactPayload_CarriesObjectName_ForTheTwoQueryFactsOnly (Assert.Same).
• VERIFIED — Attach still appends ranked last over a JsonObject — FactRankedPayload.cs:109-112, asserted by output in FactPayload_AttachSubjectThenAttach_KeepsRankedLast_AndTheSubjectOnTheWire.
• VERIFIED — the card text names the queries for both facts — FactAdvice.cs:1640, :1726 via NamedQueriesSentence (:1661-1666), which returns empty when nothing ranked, so a store replay degrades instead of throwing.
• VERIFIED — both SKUs end ORDER BY worker_ratio DESC, database_name, query_hash, query_plan_hash — PgFactCollector.QueryPerf.cs:253, DuckDbFactCollector.QueryPerf.cs:270; ParameterSensitivity_TiedRatios_AreNamedInKeyOrder is the Lite behaviour test.
• VERIFIED — the HUGEINT fix, and Darling never affected — DuckDbFactCollector.QueryPerf.cs:120 uses the BigInteger-aware helper at BlockingPairRowQuery.cs:94-99; Darling keeps Convert.ToDouble at PgFactCollector.QueryPerf.cs:122, correct for Npgsql numeric.
• VERIFIED — ParameterSensitivityFiringSignatureParityTests does not match the new SQL — its regex (Lite.Tests/ParameterSensitivityFiringSignatureParityTests.cs:103-110) ends at the ratio predicate and never reaches the SELECT list or ORDER BY; the predicate block is untouched on both SKUs.
• UNVERIFIED — the seeded timings (158 ms Postgres, 3-5 ms DuckDB) and the two production timings — live measurements a read-only tree cannot reach.
• UNVERIFIED — "Logic ran locally against Postgres 16 in a scratch harness (both passed)" — FactsNameQueriesLiveTests.cs:88 skips unless DARLING_TEST_PG is set; needs a run with it pointed at a live store.
• UNVERIFIED — "Lite FactsNameQueriesTests … Builds locally" — needs a Windows build and test run.
• UNVERIFIED — "Darling.Tests and Lite.Tests build to 0 errors" — needs a build.
• UNVERIFIED — "The literal revert runs in CI on #6643 … Each test above should fail there" — needs that run.
• REFUTED — the comment this diff writes at FactIdentity.cs:72-75, that the newly added keys are rostered "not because an edge reaches them today". Six edges reach them: RelationshipGraph.cs:198, :208, :403, :440 into QUERY_SPILLS; :168, :223 into PARAMETER_SENSITIVITY. Consequence in the inline comment.

Riskiest lines

1. PerformanceMonitor.Analysis/FactIdentity.cs:83-84 — the only change that alters findings this PR is not about: InferenceEngine.cs:356 into FactAdvice.cs:355 appends a Behind it: … sentence to the ROOT card of any story consuming either query fact as a hop, and that text is frozen into the persisted StoryText the viewer, the alert e-mail and both MCP tools render. UNCOVERED — nothing builds such a story and asserts the root's investigation; StoryNamedHopsTests exercises Describe/Sentence in isolation and cannot fail on a wrong roster decision.
2. Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.QueryPerf.cs:308-310 — three new positional ordinals against a SELECT list extended in the same hunk; an off-by-one ships the plan hash as the database on every parameter-sensitivity card. UNCOVERED on this SKU — FactsNameQueriesLiveTests.ParameterSensitivity_NamesTheOffenders_WorstFirst_WithinTheCap fails on it but skips without DARLING_TEST_PG; Lite's twin pins Lite's SELECT list, not this one.
3. Lite/Analysis/DuckDbFactCollector.QueryPerf.cs:109-123 — reader closed mid-method, a second command on the same DuckDB connection under the held read lock, then a HUGEINT conversion; this already cost every QUERY_STATS fact once (run 39981348396) because the block sits inside the catch that degrades to "no facts", so a throw here takes QUERY_HIGH_DOP with it. Covered by FactsNameQueriesTests.QuerySpills_NamesTheSpillingQueries_MostSpillsFirst_WithinTheCap.

Traps near this diff

• "Lite and Darling ship in parity … a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply: both readers (DuckDbFactCollector.QueryPerf.cs:18, PgFactCollector.QueryPerf.cs:68), both tools' descriptions (McpAnalysisTools.cs:762,771, DarlingMcpTools.cs:411,420), shared advice (FactAdvice.cs:1661).
• "Ruling (#5833): tests are load-bearing … never answer a trap with a test that reads source text" — applies to the edited pin only: FactRankedTests.cs:154-155 is updated, not added. All three new files assert behaviour; FactsNameQueriesAdviceTests.cs:78-87 re-checks the tool wiring by output.
• "A count taken off the page and named as the population's" — does not apply: spilling_query_count comes from the COUNT(DISTINCT …) above the LIMIT (PgFactCollector.QueryPerf.cs:54-55), offender_count from the full scan (:323).
• "Truncation inferred from a full page" — does not apply: no truncation flag is derived from the ranked list; the cap is stated as "capped at three" in both descriptions.
• "ORDER BY 3 + 4 DESC … Sort by named columns" — does not apply, and the diff moves toward it: both new orderings name their columns (PgFactCollector.QueryPerf.cs:80, :253).
• "FactAdvice.PopulateStoryText runs BEFORE ClusterIntoIncidents / StampClusters / Reconcile" — does not apply: both sentences are appended inside the composers (FactAdvice.cs:1640, :1726), before the blob is frozen.
• "Two copies of one definition pinned EQUAL to each other … Alias, do not copy." — does not apply: one copy per SKU in two dialects, this repo's standing shape, and no test pins the texts equal.
• "A delta family that stores no interval cannot tell a restart's zero from a quiet interval" — does not apply: no new delta column; delta_spills is already clamped at CollectorDeltaCalculator.cs:596.

Divergence (blind read vs PR body)

One, and it sits in the diff rather than the body. The body treats the FactIdentity roster edit as a naming nicety; the comment at FactIdentity.cs:72-75 goes further and tells the next reader no edge reaches these keys today. The graph says otherwise in six places, so the edit changes the composed investigation text of root cards for MEMORY_GRANT_PENDING, RESOURCE_SEMAPHORE, TEMPDB_USAGE and CPU_SPIKE stories — persisted text on four surfaces. Neither the comment nor the body says so, and no test would fail if that turned out to be unwanted. Fix the sentence and say it in the body; everything else held up on every claim graded above.

Two smaller things the body omits, both non-blocking and both inline: PgFactCollector.AllSql's "every query this collector executes" no longer covers the new statement (it would pass every pin it faces, so this is forward-looking), and a NULL database_name or query_hash renders a name like Sales/ or / into the card prose.

Reviewer checklist

• Run the Darling halves with DARLING_TEST_PG set: FactsNameQueriesLiveTests is the only thing executing the new Postgres SELECT list, its ordinals and the tie-break.
• On a live SQL Server with grant pressure, call analyze_server and read what the MEMORY_GRANT_PENDING / TEMPDB_USAGE root cards now say — whether the new Behind it: sentence reads well there is a judgement only a real card settles.
• Confirm the #6643 revert run actually fails the three new test classes.
• Re-time the ranked read on a store with thousands of distinct spilling queries: the GROUP BY runs before the LIMIT 3, and the production timings are from the servers with the most spills, not the most distinct hashes.

[erikdarlingdata]

Claude posting for Erik Darling.

Pushed afe2d7d (merged current dev). Draft state unchanged. Nothing was run locally beyond the two builds (0 errors each), because the test hosts here lack the Windows desktop runtime. CI will run the tests.

REFUTED comment at FactIdentity.cs:72-75: the reviewer is right and the comment was wrong. Both keys are hops today. The roster entries stay, because naming the query on the root card of a grant, tempdb or CPU story is what #6640 asks for (the agent in the report could not find the query from the story). The comment now names the edges. The PR body says root cards gain the sentence. New test MemoryGrantPendingRootConsumingQuerySpills_NamesTheSpillingQueryOnTheRootCard builds that story through the real engine and asserts the root's investigation names Sales/0xAB. It fails without the roster entry, since NamedHops would be empty. I could not run the revert here, so that rests on CI.

NULL names (inline): one shared FactRanked.QueryName builds the name in both collectors, with (no database), (no hash) and (no plan hash) for empty parts. The SQL identity is untouched. QueryName_ReadsANullPartAsAPlaceholder_NotAnEmptyOne pins it.

AllSql comment (inline): added QuerySpillsRankedSql to AllSql so the dialect pins cover it, and moved the count pin to +5 with its comment.

PG ordinals (inline): re-read 8, 9 and 10 against the SELECT list (database_name, query_hash, query_plan_hash). They match. FactsNameQueriesLiveTests runs them in CI where DARLING_TEST_PG is set, and a comment at the read says so.

DuckDB block (riskiest line 3): it could cost the other QUERY_STATS facts, since it sat inside the method's catch. The ranked read now has its own try in both SKUs. A throw clears the names and logs without marking the family unread.

Live-test coverage shape (inline): acknowledged. The Darling collector half rests on the live run. No Darling tie test was added.

Claude posting for Erik Darling. Answered at afe2d7d: the roster comment is corrected and the root-card effect is kept on purpose (#6640), with a behaviour test; NULL name parts, AllSql and the DuckDB try are fixed. Details in the PR comment.

[claude]

Blind-read note (non-blocking), on one claim in the body: "The Fact.Ranked doc no longer says no SQL Server collector sets it."

That is true of the doc the claim names — AnalysisModels.cs:84-95 now reads "the only SQL Server collectors that set it are the two query facts". Two other copies of the same sentence survive the diff and now state the opposite of what the tree does:

• PerformanceMonitor.Analysis/FactAdvice.cs:69 — "every fact that ranks nothing (nearly all of them, and every SQL Server fact — no SQL Server collector sets Fact.Ranked) renders the characters it rendered before", in NameTheRest's summary. The byte-identity argument it is making is still correct (neither new family calls NameTheRest); the parenthetical justifying it is not.
• Darling/Darling.Tests/FactRankedTests.cs:102-104 — "a card whose fact ranks nothing — every SQL Server fact, and every PostgreSQL family other than the three migrated ones", in ANonRankingCard_GetsItsOwnObjectBack_WithNoRankedKeyAndNoNull's summary. The test itself is unaffected (it builds its own card), only the sentence.

Both are prose, nothing executes on them, so this is a comment and not a verdict. But #6640's whole mechanism is "a reader finds the name where the doc says it is", and these two are exactly where a later lane would go looking for "does any SQL Server family rank?".

Two smaller observations I checked and am NOT raising as findings, recorded so the next reviewer does not re-derive them:

• QuerySpillsRankedSql's GROUP BY v.database_name, v.query_hash and the aggregate's COUNT(DISTINCT COALESCE(database_name,'') || '/' || COALESCE(query_hash,'')) agree on NULLs — GROUP BY treats NULLs as one group, which is the same collapse the COALESCE performs — so the comment's "same (database, query_hash) identity as the aggregate above" holds, and two ranked entries cannot collide on (no database)/(no hash) unless a row carries a literal empty string where another carries NULL, which the DMV path does not produce.
• "ranked[0].value is at most value" holds because delta_spills can never be negative: CollectorDeltaCalculator.Core reports a counter reset as 0 with no interval (CollectorDeltaCalculator.cs:570-575), never a negative difference, so the per-query sums over delta_spills > 0 cannot exceed SUM(delta_spills) over the same window.

[claude]

Verdict: LGTM

Claims

• VERIFIED — PARAMETER_SENSITIVITY sets ObjectName/Ranked off a read extended with database_name, query_hash, query_plan_hash; ordinals 8/9/10 are the last three of each SELECT list and Value/Metadata are untouched — Lite/Analysis/DuckDbFactCollector.QueryPerf.cs:270-272,326-341,354; Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.QueryPerf.cs:252-254,315-333,347.
• VERIFIED — QUERY_SPILLS ranks (database, query_hash) by pages, top three, on a second statement on the same connection, only when the aggregate found spills, each in its own try with record: false — DuckDbFactCollector.QueryPerf.cs:18-31,106-133; PgFactCollector.QueryPerf.cs:68-81,109-134. The Lite CI fix reads the HUGEINT sum with ToInt64 (:124, unwrapped at BlockingPairRowQuery.cs:94-99).
• VERIFIED — Value stays the window total and ranked[0].value is at most value: delta_spills cannot go negative (CollectorDeltaCalculator.cs:570-575 reports a reset as 0), so per-query sums over delta_spills > 0 cannot exceed SUM(delta_spills). Pinned by FactsNameQueriesTests.QuerySpills_NamesTheSpillingQueries_MostSpillsFirst_WithinTheCap.
• VERIFIED — both cards' Investigation names the queries — FactAdvice.cs:1640,1726 via NamedQueriesSentence (:1661-1666), which unlike NameTheRestList includes entry [0]; both FactsNameQueriesAdviceTests advice tests assert the wording.
• VERIFIED — AttachSubject adds object_name for these two keys only, every other fact gets its own reference back, both tools call it, ranked stays last, and no migration rung exists — FactRankedPayload.cs:77-95; McpAnalysisTools.cs:362,388; DarlingMcpTools.cs:469,478; InferenceEngine.cs:383. Pinned by both FactPayload_* tests.
• VERIFIED — the two keys joined FactIdentity.SqlServerKeys with Describe arms and both are reachable hops, so a root card gains the "Behind it" sentence — FactIdentity.cs:87-88,163-166; RelationshipGraph.cs:168,198,208,223,403,440; StoryNamedHopsTests.MemoryGrantPendingRootConsumingQuerySpills_NamesTheSpillingQueryOnTheRootCard runs the real engine and graph. Both SKUs' tiebreaks landed (DuckDbFactCollector.QueryPerf.cs:280, PgFactCollector.QueryPerf.cs:262) without disturbing ParameterSensitivityFiringSignatureParityTests' canonical block.
• UNVERIFIED — every timing in the body (158 ms vs a 285-317 ms aggregate on PG 16, 3-5 ms on DuckDB, and the two production-store pairs) — live measurements a read-only tree cannot reproduce.
• UNVERIFIED — "Logic ran locally against Postgres 16 in a scratch harness (both passed)" and "builds to 0 errors" — FactsNameQueriesLiveTests skips unless DARLING_TEST_PG is set (:199-200), and a build is not something this tree can run.
• UNVERIFIED — the #6643 revert proofs, in particular the tiebreak one: without the tiebreak DuckDB's order among equal worker_ratio rows is unspecified rather than guaranteed insert order, so whether ParameterSensitivity_TiedRatios_AreNamedInKeyOrder fails on the revert is empirical, not shown by the source.

Riskiest lines

1. Darling/PerformanceMonitor.Darling.Analysis/PgFactCollector.QueryPerf.cs:319-321 — three new positional ordinals; an off-by-one names the wrong query or throws on every Darling pass. Covered by FactsNameQueriesLiveTests.ParameterSensitivity_NamesTheOffenders_WorstFirst_WithinTheCap, which skips without DARLING_TEST_PG — UNCOVERED in a run with no live Postgres.
2. Lite/Analysis/DuckDbFactCollector.QueryPerf.cs:109 — closing the aggregate reader to run a second command on the same DuckDB connection inside the held read lock; a throw here cost QUERY_SPILLS and QUERY_HIGH_DOP in run 39981348396. Covered by FactsNameQueriesTests.QuerySpills_NamesTheSpillingQueries_MostSpillsFirst_WithinTheCap, which asserts the ranked list and the aggregate metadata together.
3. PerformanceMonitor.Analysis/FactRankedPayload.cs:88-93 — the key gate plus re-serialize-and-append; a widened predicate changes get_analysis_facts bytes for every fact in the product. Covered by FactPayload_CarriesObjectName_ForTheTwoQueryFactsOnly (its reference-identity arm fails as soon as another key serializes).

Traps near this diff

• "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — does not apply. The two QuerySpillsRankedSql bodies are per-SKU dialect copies pinned equal by no test, and the part that must not drift is aliased into FactRanked.QueryName (AnalysisModels.cs:908), called from both (DuckDbFactCollector.QueryPerf.cs:125, PgFactCollector.QueryPerf.cs:126).
• "Lite and Darling ship in parity … a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit." — does not apply. Collectors, both tool descriptions (McpAnalysisTools.cs:850,859 / DarlingMcpTools.cs:450,459) and both AttachSubject call sites are paired.
• "A count taken off the page and named as the population's … the page's count is *_returned." — does not apply. spilling_query_count still comes from the un-LIMITed aggregate (DuckDbFactCollector.QueryPerf.cs:149), so the card says "4 distinct queries" while naming three.
• "InferenceEngine.BuildStories consumes facts at or above 0.5 as their own roots BEFORE lower-severity roots walk." — does not apply, and the new story test respects it: StoryNamedHopsTests holds QUERY_SPILLS at 0.40 so the traversal keeps it as a hop.
• "Never add one, and never answer a trap with a test that reads source text." — applies to the surrounding censuses, not to this diff's new tests: the two source-reading tests touched (FactRankedTests.cs:154-155, PgFactCollectorTests.cs:119-120) are pre-existing pins kept green, and every new test here is a behaviour test.

Divergence (blind read vs PR body)

1. "The Fact.Ranked doc no longer says no SQL Server collector sets it." That doc was updated (AnalysisModels.cs:84-95), but two other copies of the sentence survive and now state the opposite of what the tree does: FactAdvice.cs:69 and Darling/Darling.Tests/FactRankedTests.cs:102-104. Prose only; posted as a comment.
2. "Considered, not added" says an AssertInvariant call on the two new facts "would fail it by design". True for QUERY_SPILLS (FactRankedTests.cs:52 asserts Value == Ranked[0].Value), but PARAMETER_SENSITIVITY satisfies the invariant in full — Value is worstRatio and ranked[0].Value is the same ordinal-2 read on the same first kept row. The shared helper is now the one place that does not know about the documented exception. Deliberate per the body, so it stays a note.
3. Unstated rather than contradicted: when the ranked read throws, spillRanked is cleared and ObjectName stays null (DuckDbFactCollector.QueryPerf.cs:130,143), so AttachSubject falls through to the reference-identity arm and the card is byte-identical to the pre-#6640 card.

Reviewer checklist

• Run FactsNameQueriesLiveTests with DARLING_TEST_PG against a real Postgres 16 store — the only thing exercising Darling's ordinals 8-10 and the Convert.ToDouble over whatever Npgsql returns for SUM(delta_spills).
• Confirm Lite.Tests passes on the Windows desktop runtime; FactsNameQueriesTests is the only behaviour coverage for the Lite collector changes.
• Check the #6643 revert run turns ParameterSensitivity_TiedRatios_AreNamedInKeyOrder red — without the tiebreak DuckDB's order among equal ratios is unspecified, so it could pass by luck.
• Read one real QUERY_SPILLS card on a busy server: Most spilled: Sales/0xAB (1,234 pages (9.6 MB)), … nests a parenthetical inside a parenthetical.
• Watch one analysis pass's wall clock on the largest production store — the ranked read is a second full group-by over the window's query_stats rows and fires on every pass where anything spilled.

[erikdarlingdata]

Claude posting for Erik Darling.

CI at ff932e1 failed one test per SKU, and 2e8c923 fixes both.

• Failure: AnalysisPassTokenThreadingTests.EveryCatchOnTheFactCollectorLetsAnAbandonmentThrough states how many fact-collector catches classify the abandonment, so that removing one is a decision. Darling expected 29 and found 30 (test leg 12). Lite expected 30 and found 31 (test leg 14).
• Cause: the new spill ranking read that names the queries behind QUERY_SPILLS has its own catch, in each SKU. Both catches classify correctly: when (!AnalysisShutdown.IsExpectedAbandon(...)) in Darling and when (!AnalysisAbandon.IsExpected(...)) in Lite. So the half of the test that looks for bare catches passed.
• Fix: the counts are now 30 and 31, with a line for Analysis: PARAMETER_SENSITIVITY and QUERY_SPILLS facts name the queries behind them #6640 in each count's history comment. No product code changed. These were the only failures across the 16 test legs.
• Darling.Tests and Lite.Tests build with 0 errors.

[claude]

Non-blocking, doc drift in the file this hunk lands in. NamedQueriesSentence is added 13 lines below two statements that this change makes false, and neither was updated:

• line 68: "every fact that ranks nothing (nearly all of them, and every SQL Server fact — no SQL Server collector sets Fact.Ranked)" — two SQL Server collectors now do (DuckDbFactCollector.QueryPerf.cs:144 / :355, PgFactCollector.QueryPerf.cs:145 / :348).
• lines 81–82: "Today's three callers are all PostgreSQL-target." — NamedQueriesSentence is a fourth reader of the seam and both its callers are SQL Server families.

The PR body says "The Fact.Ranked doc no longer says no SQL Server collector sets it", and that is true of AnalysisModels.cs:88 — but the same sentence survives here and in Darling/Darling.Tests/FactRankedTests.cs:102 ("every SQL Server fact", on the byte-identity test's summary). Three copies of one claim, two of them now wrong, is the shape .github/REVIEW-TRAPS.md names under "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted" — here they are prose rather than pins, so nothing goes red; it just misleads the next reader of this exact seam.

[claude]

Verdict: LGTM

Claims

• VERIFIED - PARAMETER_SENSITIVITY sets ObjectName/Ranked, name database/query_hash/query_plan_hash, Value the worker ratio, Figures the four keys, Metadata unchanged; the three new columns end each SELECT list and the reader takes them at ordinals 8/9/10 - DuckDbFactCollector.QueryPerf.cs:270 vs :328; PgFactCollector.QueryPerf.cs:252 vs :319.
• VERIFIED - QUERY_SPILLS runs a second statement on the same connection, grouped (database_name, query_hash), ordered by pages, capped at FactRanked.MaxObjects, only when the aggregate found spills - DuckDbFactCollector.QueryPerf.cs:18/:107; PgFactCollector.QueryPerf.cs:68/:110.
• VERIFIED - Value stays the window total and ranked[0].value is at most value (:141; delta_spills cannot go negative, QueryStatsCollector.cs:729-760), pinned by FactsNameQueriesTests.QuerySpills_NamesTheSpillingQueries_MostSpillsFirst_WithinTheCap.
• VERIFIED - aggregate figures untouched: spilling_query_count (PgFactCollector.QueryPerf.cs:54) and offender_count (:351) are still population counts, never the capped list's length.
• VERIFIED - the card text names the queries for both facts (FactAdvice.cs:1640, :1726 via :1661; FactsNameQueriesAdviceTests), including entry [0], which is what covers the one-query case where analyze_server's root_fact carries no name.
• VERIFIED - AttachSubject adds object_name for these two keys only, other facts keep their bytes by reference return, both tool files call it, ranked still lands last - FactRankedPayload.cs:77-95; McpAnalysisTools.cs:362; DarlingMcpTools.cs:486; FactPayload_CarriesObjectName_ForTheTwoQueryFactsOnly.
• VERIFIED - Lite has the same change, no migration rung (AnalysisModels.cs:88); the invariant exception is stated at AnalysisModels.cs:11-15 and in both get_analysis_facts descriptions, and the SKUs' four tool descriptions are byte-identical.
• VERIFIED - the CI fix reads DuckDB's SUM(BIGINT) through the BigInteger-aware ToInt64 (DuckDbFactCollector.QueryPerf.cs:124, BlockingPairRowQuery.cs:94); Darling keeps Convert.ToDouble over Npgsql's numeric (PgFactCollector.QueryPerf.cs:125), correct for that provider.
• VERIFIED - FactRankedTests.BothToolFiles_ updated to the AttachSubject shape with the tool files unchanged (:150); the four other source pins the body names do not match the new SQL or code, and ArchiveTieOrderTests reads only offender_count/worst_ratio.
• VERIFIED - both facts are hops of the named roots today (RelationshipGraph.cs:168, 198, 208, 223, 403, 440); roster and Describe arms at FactIdentity.cs:87/:163, roster test updated at StoryNamedHopsTests.cs:36. root_fact still carries no object_name on analyze_server (McpAnalysisTools.cs:217), as declined.
• VERIFIED - both SKUs end ORDER BY worker_ratio DESC, database_name, query_hash, query_plan_hash (:280; :262), pinned by ParameterSensitivity_TiedRatios_AreNamedInKeyOrder.
• VERIFIED - the census bumps land on real counts: 30 classified catches in PgFactCollector., 31 in DuckDbFactCollector., AllSql 37 = 32 + 5, and the new SQL clears the AllSql dialect pins (PgFactCollectorTests.cs:222).
• UNVERIFIED - the seeded timings and seed shape (1M rows, 20k queries, ~14% spilling) - a live measurement.
• UNVERIFIED - the two production timings - measured on named stores; nothing in the repo records them.
• UNVERIFIED - "logic ran locally against Postgres 16 in a scratch harness" - FactsNameQueriesLiveTests is Assert.SkipWhen on DARLING_TEST_PG (:217) and only nightly.yml sets it.
• UNVERIFIED - "Darling.Tests and Lite.Tests build to 0 errors" - dotnet build was unavailable in this sandbox, so every compile-level claim rests on CI.
• UNVERIFIED - the #6643 revert run - promised, not yet in the body.
• UNVERIFIED - the CI-fix root-cause narrative (run 39981348396, the deleted repro console) - the tree's fix matches the mechanism; the history is not checkable.

Riskiest lines

1. PgFactCollector.QueryPerf.cs:319 - three columns read by positional ordinal; an off-by-one names the wrong query in object_name, ranked and the new "Behind it:" sentence, with no exception to notice - UNCOVERED on PR CI (FactsNameQueriesLiveTests.ParameterSensitivity_NamesTheOffenders_WorstFirst_WithinTheCap skips unless DARLING_TEST_PG is set; only nightly.yml sets it).
2. DuckDbFactCollector.QueryPerf.cs:109 - reader.CloseAsync() now sits above both totalSpills blocks and above QUERY_HIGH_DOP; a read off reader past it throws into the outer catch and costs the whole family - covered by QueryHighDopStaleMaxDopTests (same SELECT, :101) with QuerySpills_NamesTheSpillingQueries_MostSpillsFirst_WithinTheCap.
3. DuckDbFactCollector.QueryPerf.cs:124 - the HUGEINT read that already cost every QUERY_STATS fact once - covered by QuerySpills_NamesTheSpillingQueries_MostSpillsFirst_WithinTheCap, which runs it against a real DuckDB.

Traps near this diff

• "a bare integer in ORDER BY is an output-column ordinal, but 3 + 4 is an expression ... Sort by named columns." - does not apply - both new sorts name columns (PgFactCollector.QueryPerf.cs:80, :262).
• "A percent whose denominator is the PAGE ... Ask what a share is of." - does not apply - no share; each entry carries its own absolute SUM(delta_spills) (:72) and the total stays the window aggregate (:148).
• "A count taken off the page and named as the population's." - does not apply - :54 and :351 are population counts, not the capped list's length.
• "Truncation inferred from a full page ... the flag is fetched.Count > limit." - does not apply - nothing infers truncation; the cap is declared (AnalysisModels.cs:46).
• "PopulateStoryText runs BEFORE ClusterIntoIncidents / StampClusters / Reconcile, so a sentence added after folding must re-read the frozen StoryText." - does not apply - the prose is composed inside the two composers (FactAdvice.cs:1640, :1726) and the hop sentence rides the existing path (FactIdentity.cs:211).
• "BuildStories consumes facts at or above 0.5 as their own roots BEFORE lower-severity roots walk." - applies, honoured - QUERY_SPILLS is seeded at 0.4 (StoryNamedHopsTests.cs:231).
• "A key in the graph is not a key in the data; grep the collector that emits it." - applies to the roster comment's edge list, checks out - every edge named exists and both keys are emitted by both SKUs.
• "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted. Alias, do not copy." - applies weakly - two engines, two statements; the shareable piece is shared (FactRanked.QueryName, AnalysisModels.cs:56). The prose half is my inline comment.
• "Lite and Darling ship in parity: a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit." - applies, satisfied - both collectors, both tool files, four identical descriptions.
• "never answer a trap with a test that reads source text." - applies, respected - the new coverage is behaviour; the source pins edited are pre-existing censuses kept in step.
• "InternalsVisibleTo on PerformanceMonitor.Analysis names Lite.Tests only - a helper Darling.Tests must call has to be public." - applies, satisfied - AttachSubject and QueryName are both public.

Divergence (blind read vs PR body)

One, narrow. The body says "The Fact.Ranked doc no longer says no SQL Server collector sets it." True of AnalysisModels.cs:88, but the same claim survives at FactAdvice.cs:68, thirteen lines above the new NamedQueriesSentence, at FactAdvice.cs:81-82 ("Today's three callers are all PostgreSQL-target"), and at Darling/Darling.Tests/FactRankedTests.cs:102. Prose only, nothing goes red; left as the one non-blocking inline comment. Otherwise the accounts agree, and twice the body is ahead of a blind read: "Considered, not added" pre-empts the AssertInvariant gap I reached independently, and "Root cards gain the query name" discloses the roster's effect on frozen StoryText, which "What changed" alone would not have led a reader to.

Reviewer checklist

• Run the Darling suite with DARLING_TEST_PG set and confirm FactsNameQueriesLiveTests executes rather than skips; PR CI sets it nowhere, so riskiest line 1 lands unexecuted.
• Build and run Darling.Tests + Lite.Tests; dotnet build was unavailable here.
• Post the #6643 revert-run results; until then the revert proof for all seven new tests is a claim about a run nobody can read.
• Re-time the ranked read on a store with thousands of spilling queries: the bound is the GROUP BY over the window, not the LIMIT 3 the body reasons from.
• Read one real QUERY_SPILLS and one PARAMETER_SENSITIVITY card in the viewer and in an e-mail body: the sentence nests parentheses, and the label reads "Worst plans" even when one plan rode.

[erikdarlingdata]

Claude posting for Erik Darling.

Revert proof, recorded after the merge. #6643 undid this PR's code on dev at c114110 and kept its tests: the Darling and Lite collectors set no query names on PARAMETER_SENSITIVITY and QUERY_SPILLS, the card text names no queries, and the fact payload adds no object_name. CI run 40132354137 at 1b6162b failed 9 of the 11 new tests:

• Lite (FactsNameQueriesTests, test leg 14): ParameterSensitivity_NamesTheOffenders_WorstFirst_WithinTheCap, ParameterSensitivity_TiedRatios_AreNamedInKeyOrder and QuerySpills_NamesTheSpillingQueries_MostSpillsFirst_WithinTheCap.
• Darling live (FactsNameQueriesLiveTests, test leg 2, against PostgreSQL): ParameterSensitivity_NamesTheOffenders_WorstFirst_WithinTheCap and QuerySpills_NamesTheSpillingQueries_MostSpillsFirst_WithinTheCap.
• Darling (FactsNameQueriesAdviceTests, test leg 6): QuerySpillsAdvice_NamesTheSpillingQueries, ParameterSensitivityAdvice_NamesThePlans, FactPayload_CarriesObjectName_ForTheTwoQueryFactsOnly and FactPayload_AttachSubjectThenAttach_KeepsRankedLast_AndTheSubjectOnTheWire.

Two new tests passed, as expected, because the revert leaves their code in place:

• QueryName_ReadsANullPartAsAPlaceholder_NotAnEmptyOne tests the name helper.
• StoryNamedHopsTests.MemoryGrantPendingRootConsumingQuerySpills_NamesTheSpillingQueryOnTheRootCard tests the roster entries, and the revert does not touch the roster.

No other test failed. Darling.Tests failed only these 6, and Lite.Tests failed only these 3. #6643 is now closed and its branch deleted.