[erikdarlingdata]

Claude posting for Erik Darling.

Refs #6598.

get_query_store_query_history now reports a removed force. Before, a plan forced when sampled showed is_forced_plan: true, forced_state: null and forced_plans: []. The agent needed a second call to get_forced_plans to learn the force was REMOVED.

What changed

• TryReadForcedPlansAsync keeps REMOVED facts. They show in forced_plans and as forced_state: "REMOVED" on the plan.
• One fact per plan comes from the reader's DISTINCT ON (database_name, plan_id) (DarlingForcedPlanStateReader.cs), so a re-force cannot duplicate. PickForces keeps the named database's facts only (a blank database_name widens the read to every database) and sorts standing forces before REMOVED ones, ahead of the Take(MaxPlans) cap. It does no per-plan choice.
• Standing forces sort before REMOVED ones, so REMOVED never pushes a standing force out of the 20 plan cap.
• is_forced_plan stays "forced when sampled, or standing now" (IsForced). A REMOVED fact alone does not set it. The synthesized row for a never-run forced plan uses the same IsStanding test. The web History panel is the one reader of forced_state. It now reads No (removed) for a removed force that was never sampled as forced.
• The tool description now says REMOVED is listed, and that forced_state null with status ok means no force is in the capture. The head (before the guide marker) is untouched, so the 160 character budget test is not affected. No test pinned the old wording.
• Lite has no twin of this tool, so there is nothing to mirror. No migration rung, no new tool.

Tests and revert proofs

Test	Revert that fails it	Status
TheHistoryTool_ListsAForcedPlanWithNoExecutionsInTheWindow_AndARemovedOneWithItsState (live PG, plan 51 sampled as forced, REMOVED fact, now also null runs beside 3 plan executions)	Put the State != Removed filter back, forced_state reads null	waits for CI
PickForces_KeepsTheNamedQuerysFacts_OnePerPlan (the reader's shape, standing before REMOVED)	Sort by plan id only	waits for CI
WithForcedPlans_AForceWithNoRows_GetsARowThatIsForcedOnlyWhileItStands	Invert IsStanding on the synthesized row	waits for CI
ForceNode_ARemovedForceDoesNotReportZeroRuns_AStandingOneDoes	Drop the Removed check in ForceNode, the REMOVED node reads 0 again	waits for CI
PickForces_ListsStandingForcesBeforeRemovedOnes	Sort by plan id only, plan 99 falls past the cap	waits for CI
IsForced_IsSampledOrStanding_AndARemovedFactAloneIsNeither	Make a REMOVED fact set the flag	waits for CI
TheForcedColumn_ReadsARemovedForceAsNoRemoved_AndSpacesEveryUnderscoreInAStateName (Node harness, rendered row text)	Old label: Yes (REMOVED) for the first row and unknown plan_not_collected for the second, so both assertions fail	ran locally, passes

Review round

• UI "Yes (removed)": real. forcedLabel in query-store-history.js follows is_forced_plan, so it shows No (removed). The regex also fixes UNKNOWN_PLAN_NOT_COLLECTED: it read unknown plan_not_collected and now reads unknown plan not collected. TheForcedColumn_ReadsARemovedForceAsNoRemoved_AndSpacesEveryUnderscoreInAStateName renders the panel under Node and checks both row texts.
• PickForces database grouping: real. It now takes databaseName and drops other databases. PickForces_KeepsOnlyTheNamedDatabase fails if the database filter is removed. Waits for CI.
• Re-force crash claim: body was wrong. Corrected above.
• Consumer survey: body was wrong. Corrected above.
• Line 399 second copy of "standing": real. Both uses call IsStanding.
• Fixture with two facts for one plan in one database: real, the reader's DISTINCT ON (database_name, plan_id) cannot emit it. Once the database filter has run there is one fact per plan, so the per-plan group and its standing-beats-REMOVED tiebreak were dead code. Removed, and the test now uses facts the reader emits. PickForces_KeepsOnlyTheNamedDatabase carries the cross-database shape.
• REMOVED evidence as zeros: real. A REMOVED force's runs are not read, so ForceNode now emits runs_read: false and null forced_plan, variant_plans, other_plans, unknown_plans and their totals. get_forced_plans shares the node, so it changes the same way. Both descriptions say null means not measured.
• Uncovered synthesized row: the row logic is now WithForcedPlans, and a test pins IsStanding for a REMOVED and a standing force with no rows.
• Misplaced scope comment and "standing forces first" wording: moved.
• UNVERIFIED claims: tests build on macOS, not run here. They wait for CI with DARLING_TEST_PG set.
• Considered, not added: a truncation flag on forced_plans. The cap and its lack of a flag are older than this PR, and 20 forces on one query has no known trigger.

Darling.Tests builds on macOS but cannot run here, and no PostgreSQL was available. Nothing was run locally beyond the build. This draft stays a draft until CI is started.

CHANGELOG entry

SECTION: Fixed
ENTRY:

• Query history now says when a plan's force was removed ([Darling MCP: get_query_store_query_history says a plan is forced but drops its REMOVED force, so it disagrees with get_forced_plans #6598]) - get_query_store_query_history listed a removed force as forced with no state and an empty forced_plans. It now lists the force with state REMOVED and keeps standing forces first. A REMOVED force's run counts were never read, so get_forced_plans and the history tool now return them as null with runs_read: false instead of 0. The web History panel Forced column reads No (removed), and UNKNOWN_PLAN_NOT_COLLECTED now reads unknown plan not collected.

[claude]

The one reader of forced_state was not updated: the web Forced column now says "Yes (removed)" for a plan that is not forced.

forced_state has exactly one consumer outside the tests, the Query Store History panel this tool was built for (wwwroot/js/pages/query-store-history.js:158, reading readTool("get_query_store_query_history") at line 108):

cell(p.forced_state ? "Yes (" + p.forced_state.toLowerCase().replace("_", " ") + ")" : p.is_forced_plan ? "Yes" : "No"),

is_forced_plan is only consulted when forced_state is null. Before this PR a removed force reached that cell as forced_state: null, so the column read No (or Yes, when the plan was sampled forced). After this PR the same plan arrives as forced_state: "REMOVED", so the cell renders Yes (removed) — and for a plan that was never sampled as forced the payload beside it says is_forced_plan: false. The Forced column and the field the column is named after now contradict each other, which is the opposite of what the new description sentence promises ("it does not make is_forced_plan true").

The payload change is right; the cell needs to stop treating "has a state" as "is forced", e.g. render the state as a qualifier on the is_forced_plan answer (No (force removed) / Yes (working)) rather than hard-coding Yes (.

Nothing fails on this today: WebQueryStoreHistoryColumnTests (Darling/Darling.Tests/WebQueryStoreHistoryColumnTests.cs:23-41) is a source pin — it asserts the column's sortable/filter/csv/copy flags and that no innerHTML is used — so it passes whatever this cell renders.

[claude]

The re-force mechanism the PR body gives for the group step is not real, and the duplicate the group step does collapse is one it collapses wrongly.

The body says: "The forced-plan read can return both for one plan after a re-force, and ToDictionary would have thrown." The read cannot do that. DarlingForcedPlanStateReader.ForcesSql's last CTE is

latest AS (
    SELECT DISTINCT ON (database_name, plan_id) ...
    FROM plans ORDER BY database_name, plan_id, collection_time DESC)

(DarlingForcedPlanStateReader.cs:196-199) and the final SELECT joins only absent (grouped by database_name, plan_id) and newest (grouped by database_name), so it emits exactly one row per (database, plan), and facts is 1:1 with those rows (DarlingForcedPlanStateReader.cs:520-578). A re-force is already folded into that single fact — the first-seen subquery at line 201 picks the current run's start, which is what FirstSeen_IsTheFirstCaptureOfTheCurrentRun_AndAForceSetAgainStartsANewRun (ForcedPlanStateLiveTests.cs:147) pins. With DatabaseFilter.One(database_name) that is one fact per plan_id, before and after this PR, so ToDictionary at line 394 was never reachable by a re-force.

Two consequences:

1. PickForces_KeepsOneFactPerPlan_AndAStandingForceBeatsARemovedOne feeds PickForces two facts for plan 7 — a population the reader cannot produce. That is the #3709 trap shape ("a fixture … testing a shape no live run produces"); the test is fine as a unit pin of the ordering rule, but it should not be cited as the revert proof for a crash that could not happen.

2. There is one path that produces duplicate plan_ids, and it is a different one: DatabaseFilter.One(name) returns All for a null/empty/whitespace name (DatabaseFilter.cs:63-64), and GetQueryStoreQueryHistory never rejects a blank database_name (lines 327-333). database_name = " " therefore reads the forces of every database for that query_id, where one plan_id in two databases is ordinary. Before this PR that threw out of ToDictionary; now PickForces groups on PlanId without DatabaseName, so it silently merges two databases' forces and reports one of them — a state for a force in a database the caller did not ask about. Either key the grouping on (DatabaseName, PlanId), or refuse a blank database_name up front.

[claude]

Non-blocking: IsForced carries two <summary> blocks, and the first one is PickForces' documentation — "One fact per plan_id for the query (#6598): a standing force wins over a REMOVED one … Standing forces come first, so a REMOVED force never pushes a standing one out of the MaxPlans cap" describes the method below, not this predicate. PickForces is left undocumented. No build break (neither GenerateDocumentationFile nor TreatWarningsAsErrors is set anywhere in the tree), but the doc block belongs on line 301.

[claude]

Non-blocking, but it is the #3684 shape ("Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted. Alias, do not copy"): f.State != ForcedPlanState.Removed here is a second, open-coded copy of the "standing" test that IsForced now owns at line 299. It is also redundant — every plan synthesized here comes from forced.Facts, so its fact is always in forcedById, and line 420 computes IsForced(p.IsForcedPlan, fact) over it anyway; passing false would give the same answer for both the standing and the REMOVED case. Dropping it back to false (or calling IsForced(false, f)) leaves one definition of "standing".

This line is also the second thing the PR body's "Nothing else in this answer counts current forces" misses — it is a change the diff makes and the "What changed" list does not mention.

[claude]

Verdict: CHANGES REQUESTED

Claims

• VERIFIED — "Before, a plan forced when sampled showed is_forced_plan: true, forced_state: null and forced_plans: []" — the pre-change TryReadForcedPlansAsync filtered State != Removed before Take(MaxPlans), and the old live assertion was Assert.Empty(...forced_plans) plus forced_state null (ForcedPlanStateLiveTests.cs:263-264, LEFT side of the diff).
• VERIFIED — "TryReadForcedPlansAsync keeps REMOVED facts; they show in forced_plans and as forced_state: "REMOVED"" — DarlingMcpQueryStoreHistoryTools.cs:283, :421, :424, with StateName mapping Removed at DarlingMcpForcedPlanTools.cs:45; asserted by TheHistoryTool_ListsAForcedPlanWithNoExecutionsInTheWindow_AndARemovedOneWithItsState.
• VERIFIED — "One fact per plan_id (PickForces); a standing force wins over REMOVED, otherwise the newest LastSeen" — :303-304; PickForces_KeepsOneFactPerPlan_AndAStandingForceBeatsARemovedOne.
• VERIFIED — "Standing forces sort before REMOVED, so REMOVED never pushes a standing force out of the 20 plan cap" — :305-306; PickForces_ListsStandingForcesBeforeRemovedOnes.
• VERIFIED — "is_forced_plan stays 'forced when sampled, or standing now'; a REMOVED fact alone does not set it" — :298-299 is logically identical to the old p.IsForcedPlan || forcedById.ContainsKey(...) over a Removed-filtered dictionary; IsForced_IsSampledOrStanding_AndARemovedFactAloneIsNeither pins all four arms.
• VERIFIED — "The head (before the guide marker) is untouched, so the 160 character budget test is not affected" — every changed description line is after <<GUIDE>> (:314-316); the test slices at IndexOf("<<GUIDE>>") (DarlingMcpQueryStoreHistoryToolsTests.cs:78-79).
• VERIFIED — "No test pinned the old wording" — the removed sentences ("a REMOVED force is left out", "the force no longer stands") appear nowhere else in the tree.
• VERIFIED — "Lite has no twin of this tool, so there is nothing to mirror" — no get_query_store_query_history under Lite/ or Lite.Tests/; docs/changelog/3.10.md:104 records the absence as deliberate.
• VERIFIED — revert proof "Sort by plan id only, plan 99 falls past the cap" — with OrderBy(PlanId) alone the 30 REMOVED facts fill MaxPlans; :305-306 is what prevents it.
• VERIFIED — "live PG, plan 51 sampled as forced" — ForcedPlanStateLiveTests.cs:112 seeds forcedWhenSampled: true, bound at :55-60; it touches only query 5, and the forced-plan reader reads collect.query_store_forced_plans, not that column, so no other assertion in the shared seed moves.
• UNVERIFIED — "Darling.Tests builds on macOS but cannot run here, and no PostgreSQL was available; nothing was run locally beyond the build" — a statement about the author's machine; it needs a build/test log.
• UNVERIFIED — the four table rows' "waits for CI" status — the live row skips without DARLING_TEST_PG (ForcedPlanStateLiveTests.cs:67), so only a CI run with Postgres shows it green.
• REFUTED — "The forced-plan read can return both for one plan after a re-force, and ToDictionary would have thrown" — ForcesSql's latest CTE is SELECT DISTINCT ON (database_name, plan_id) (DarlingForcedPlanStateReader.cs:196-199) and the outer select joins only absent (grouped on database_name, plan_id) and newest (grouped on database_name), so one row per (database, plan), and facts is 1:1 with it (:520-578). A re-force is folded into that single fact by the first-seen subquery at :201, which FirstSeen_IsTheFirstCaptureOfTheCurrentRun_AndAForceSetAgainStartsANewRun pins. Duplicates are reachable only via DatabaseFilter.One("") → All (DatabaseFilter.cs:63-64) — a different mechanism the new grouping handles wrongly (inline note on PickForces).
• REFUTED — "Nothing else in this answer counts current forces" — :399 does, and the diff changes it: the synthesized PlanTotals for a never-run forced plan went from literal true to f.State != ForcedPlanState.Removed. It is a second open-coded copy of "standing" and is absent from the "What changed" list.

Riskiest lines

1. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpQueryStoreHistoryTools.cs:421 — forced_state can now be "REMOVED", and the only non-test reader of it (wwwroot/js/pages/query-store-history.js:158) prefixes every non-null state with "Yes (", consulting is_forced_plan only when the state is null — UNCOVERED (WebQueryStoreHistoryColumnTests is a source pin)
2. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpQueryStoreHistoryTools.cs:420 — the change turns on is_forced_plan keeping its old meaning while forced_state gains a value; a sign error relabels every removed force as standing — covered by IsForced_IsSampledOrStanding_AndARemovedFactAloneIsNeither and TheHistoryTool_ListsAForcedPlanWithNoExecutionsInTheWindow_AndARemovedOneWithItsState
3. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpQueryStoreHistoryTools.cs:305 — REMOVED facts now compete for the MaxPlans cap and this ordering is the only thing keeping a standing force in the answer; forced_plans still carries no truncation flag, so a cut is invisible — covered by PickForces_ListsStandingForcesBeforeRemovedOnes

Traps near this diff

• "Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply — no counterpart exists: no get_query_store_query_history under Lite/, and docs/changelog/3.10.md:104 says "Lite's MCP server does not have it".
• "A test that constructs a 'four maintenance anomalies in one run' fixture is testing a shape no live run produces (#3709). Fixtures should carry the population the engine actually emits, or say they do not." — applies — DarlingMcpQueryStoreHistoryToolsTests.cs:37-44 hands PickForces two facts for plan 7, which DarlingForcedPlanStateReader.cs:196-199 says the reader never emits for a single database.
• "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 field this PR changes is rendered by query-store-history.js:158, whose only tests (WebQueryStoreHistoryColumnTests.cs:23-41) assert column flags, a key: string and the absence of innerHTML, so the "Yes (removed)" regression has no test that can fail on it.
• "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — applies — :399 open-codes f.State != ForcedPlanState.Removed beside IsForced's identical test at :299.
• "Truncation inferred from a full page: truncated = rows.Count >= limit" — does not apply — plans_truncated is allPlans.Count > MaxPlans over the folded population (:392), not a page length; PickForces' Take(MaxPlans) at :306 sets no flag at all (pre-existing).
• "A JSON number above 2^53 is rounded by every JavaScript client … emit 64-bit ids as strings" — does not apply — the ids here are Query Store plan_id / query_id, emitted as numbers at :412 and DarlingMcpForcedPlanTools.cs:207-208 before and after this diff.

Divergence (blind read vs PR body)

Two places, both in the body's favour.

1. The body's survey of consumers stops at the MCP payload. "Nothing else in this answer counts current forces" plus "Lite has no twin of this tool, so there is nothing to mirror" reads as "no caller needs changing". The web Query Store History panel — the surface this tool was built for (#5300) — is a caller, and the only reader of forced_state in the tree. It renders forced_state ? "Yes (" + state + ")" : is_forced_plan ? "Yes" : "No", so a plan whose force was removed and that was never sampled as forced now shows Yes (removed) in the Forced column while the same payload says is_forced_plan: false. The diff makes the payload honest and the UI dishonest in one move.

2. The crash the group step is credited with preventing was not reachable, and the one that is reachable is handled wrongly. DISTINCT ON (database_name, plan_id) rules out the re-force duplicate the body names. The duplicate that can occur comes from DatabaseFilter.One("") collapsing to All for a blank database_name the tool never rejects — and PickForces groups on PlanId alone, so in that path it merges two databases' forces into one reported state instead of throwing.

Everything else matches, including the parts easy to get wrong: IsForced really is behaviour-preserving for is_forced_plan, the description head really is untouched, and the standing-first sort really does protect a standing force from the cap.

Reviewer checklist

• Open the web Query Store grid's History panel on a query whose force was removed inside the window and read the Forced column — confirm whether it says Yes (removed) for a plan whose payload has is_forced_plan: false, and settle the wording for each of the six states (note "UNKNOWN_PLAN_NOT_COLLECTED".toLowerCase().replace("_", " ") replaces only the first underscore, so it already renders unknown plan_not_collected).
• Run Darling.Tests with DARLING_TEST_PG set — the live assertion skips silently without it, so a green run without that variable proves nothing about TheHistoryTool_…AndARemovedOneWithItsState.
• On a real store, call the tool for a query with more than 20 forces in the window (standing plus removed) and check that the forces you need survived the MaxPlans cut — forced_plans carries no truncation flag to say a cut happened.

[claude]

Verdict: LGTM

Claims

None to grade. The PR body describes only commit e8c53611 (query-history REMOVED-force reporting) and is not re-graded in this follow-up pass. The three commits actually new in this range — 3a32c4597 ("MCP setup page"), 0a6121e1c ("Fix the store script's owner list for the route functions #6593 changed"), 6641d44d2 ("Darling MCP: clear a route's agent level back to Inherit... (L5 of #6593)") — arrived via the tip commit b518c54e2, a merge of origin/dev into this branch (git log --oneline --graph be00f528..b518c54e shows b518c54e2's second parent is 6641d44d2, which chains back through 0a6121e1c and 3a32c4597). None of them is authored on this PR's branch, and the PR body makes no claim about any of them.

Riskiest lines

None in scope. Per the follow-up instructions, content a base-branch merge brings in is not the author's change and is left alone unless a new commit interacts with it. I checked for interaction: the files this range touches (Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpAlertTools.RouteAdmin.cs, DarlingMcpWritePolicy.cs, DarlingConfig.cs, Darling/tools/provision-roles.sql, Darling/Darling.Tests/McpRouteAgentFieldsLiveTests.cs, Darling/README.md, docs/mcp-server.md) have zero overlap with the files this PR's own commit e8c53611 touches (Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpQueryStoreHistoryTools.cs, Darling/Darling.Tests/ForcedPlanStateLiveTests.cs, Darling/Darling.Tests/.../DarlingMcpQueryStoreHistoryToolsTests.cs). No interaction, so no new lines of this PR's own to risk-rank.

Traps near this diff

None evaluated as findings — .github/REVIEW-TRAPS.md is a check against the author's own changed lines, and this range has none (see above). The merged-in route-agent-level and owner-list work (#6639, #6644, #6608/#6429) is reviewable on its own PRs, not here.

Divergence (blind read vs PR body)

None — the PR body's account of its own change (commit e8c53611) matches what the earlier full-pass review (head be00f5286d1c14d43f788ee381900ecbfa64039a) already checked it against, and this range adds nothing the body speaks to. Note for the record: that earlier full pass ended CHANGES REQUESTED (two refuted claims, an uncovered UI-dishonesty regression in query-store-history.js, and a PickForces grouping gap) and none of that is touched or fixed by this range's merge — this pass's LGTM reflects only "nothing new or interacting was added," not that those findings are resolved.

Reviewer checklist

• Before merging, confirm the open findings from the head=be00f5286d1c14d43f788ee381900ecbfa64039a full review (UI "Yes (removed)" regression, PickForces database-grouping gap) are addressed by a later commit that a subsequent follow-up pass actually reviews — this pass's clean verdict does not supersede that review on its merits, only on scope.

Claude posting for Erik Darling. Answered at 85a0b6d: the grid now shows "No (removed)" for a removed force that was never sampled as forced. PickForces keeps only the named database, the "standing" test has one definition, and the body is corrected. Tests wait for CI with DARLING_TEST_PG.

[claude]

A REMOVED fact's evidence block is structurally zero, and this answer now publishes it beside the plan's real executions.

DarlingForcedPlanStateReader reads runs only for the forces it calls live (DarlingForcedPlanStateReader.cs:488 var live = forces.Where(InNewest), stats at :492-511). A REMOVED force is by definition not InNewest (:483), so ran never gets an entry for it, own is default(Ran) at :570, and the fact is built with ForcedExecutions = 0, ForcedLastExecution = null, OtherPlans = [], Variants = [].

ForceNode renders those as measurements (DarlingMcpForcedPlanTools.cs:216-220): forced_plan: { execution_count: 0, last_execution_time: null }, other_plans: [], other_plans_total: 0. Before this diff the history tool dropped REMOVED facts, so it never emitted that node; now it always does for them.

The live test is the demonstration: plan 51 ran 3 times in the window (ForcedPlanStateLiveTests.cs:112), so the one answer says

plans:        [{ plan_id: 51, execution_count: 3, forced_state: "REMOVED", is_forced_plan: true }]
forced_plans: [{ plan_id: 51, state: "REMOVED", forced_plan: { execution_count: 0, last_execution_time: null }, other_plans_total: 0 }]

An agent asked "was the force doing anything before it was removed?" reads execution_count: 0 and other_plans_total: 0 as "nothing ran" — the shape .github/REVIEW-TRAPS.md names: "IsDBNull ? 0: an unmeasured pass is null with a *_measured flag beside it, not 0 MB (#3682)", and "CASE WHEN interval > 0 THEN delta / interval ELSE 0 END spells 'unknowable' as 0 … End at END (NULL) and carry the NULL to the wire."

get_forced_plans has emitted the same zeros for REMOVED forces since #5632, so the cheapest honest fix is at ForceNode (null the counters when State == Removed, which fixes both tools); the narrow one is a clause in this tool's description saying a REMOVED force's run counters are not measured. Either way the answer should not state 0 where it means "not measured".

[claude]

The body says "Fixture with two facts for one plan: the fixture now says why." This docstring does not say why, and the fixture is still the unreachable shape.

Fact(7, Removed, t+2h) and Fact(7, Working, t) both default to db: "Db", so this is two facts for one plan in one database. ForcesSql's latest CTE is SELECT DISTINCT ON (database_name, plan_id) (DarlingForcedPlanStateReader.cs:196-199) and facts is 1:1 with its rows (:520-578), so the reader cannot emit it — which means the group step and its standing-beats-REMOVED tiebreak at DarlingMcpQueryStoreHistoryTools.cs:307-308 are unreachable by construction once the database filter at :306 has run.

.github/REVIEW-TRAPS.md: "A test that constructs a 'four maintenance anomalies in one run' fixture is testing a shape no live run produces (#3709). Fixtures should carry the population the engine actually emits, or say they do not." PickForces_KeepsOnlyTheNamedDatabase carries the reachable shape (two databases), so the honest fix here is one clause in this summary — e.g. "the reader emits at most one fact per (database, plan_id), so this fixture is the group step's guard, not a shape a live read produces" — or merge the two facts into the cross-database fixture and delete the branch. The PickForces XML doc at :301-303 reads the same way: "a standing force wins over a REMOVED one, else the newest (LastSeen) wins" describes a tiebreak nothing can trigger.

[claude]

Non-blocking: "standing forces first" is attached to the sentence about the per-plan listing, and plans is not in that order — it is .OrderBy(p => p.PlanId) at :404. The standing-first order is forced_plans' (PickForces at :309). A reader that takes this sentence at face value will treat plans[0] as the standing force.

Suggest moving the clause onto the next sentence, where forced_plans is the subject — e.g. keep this line as it was and say "A force removed in the window is listed too, as forced_state REMOVED, after the standing forces in forced_plans; …". The cap argument the clause is really making ("a REMOVED force never pushes a standing one out") is about forced_plans, not about plans.

[claude]

Two non-blocking notes on this hunk.

1. replace("_", " ") → replace(/_/g, " ") is a real wording change for one of the other states: UNKNOWN_PLAN_NOT_COLLECTED rendered unknown plan_not_collected before this diff and renders unknown plan not collected after it. That is a fix (the previous review's checklist named it), but the body's "The wording for the other states is unchanged" is not what the line does — worth a word in the body or the changelog entry so the UI change is not a surprise.

2. The scope comment at :144-145 now sits above forcedLabel, which has no scope parameter; planTable at :152 does. Moving the new function above that comment, or the comment down to planTable, keeps each comment next to what it explains.

[claude]

Verdict: CHANGES REQUESTED

Claims

• VERIFIED — "Before, a sampled-forced plan showed is_forced_plan: true, forced_state: null, forced_plans: []" — the LEFT side filtered State != Removed; the old assertion was Assert.Empty(...forced_plans) (ForcedPlanStateLiveTests.cs:263-264, LEFT).
• VERIFIED — "REMOVED facts show in forced_plans and as forced_state: "REMOVED"" — DarlingMcpQueryStoreHistoryTools.cs:283, :425, :428; asserted by TheHistoryTool_ListsAForcedPlanWithNoExecutionsInTheWindow_AndARemovedOneWithItsState.
• VERIFIED — "One fact per plan_id, named database only" — :305-310; the Ordinal filter matches the read's own =/= ANY on the bare column (DarlingForcedPlanStateReader.cs:185, DatabaseFilter.cs:164-165, names never case-folded at :24-25), so it drops no row the read returned.
• VERIFIED — "The read returns one row per database and plan; duplicates came only from a blank database_name" — DISTINCT ON (database_name, plan_id) (DarlingForcedPlanStateReader.cs:196-199); DatabaseFilter.One("") is All (DatabaseFilter.cs:63-64). Corrects the earlier body, as asked.
• VERIFIED — "PickForces drops other databases, so ToDictionary cannot throw" — :306-307 feed distinct keys to :398; PickForces_KeepsOnlyTheNamedDatabase fails without the filter.
• VERIFIED — "Standing forces sort before REMOVED, so REMOVED never pushes a standing force out of the cap" — :309 before :310's Take; PickForces_ListsStandingForcesBeforeRemovedOnes fails on a plan-id-only sort.
• VERIFIED — "is_forced_plan stays 'forced when sampled, or standing now'; the synthesized row uses the same IsStanding" — IsForced at :297-298 is value-identical to the old expression over a Removed-filtered dictionary, :403 calls IsStanding; IsForced_IsSampledOrStanding_AndARemovedFactAloneIsNeither pins four arms. Closes the previous review's open-coded-copy finding (#3684).
• VERIFIED — "The web History panel is the one reader of forced_state; it now reads No (removed)" — one non-test occurrence (query-store-history.js:148); forcedLabel :147-149 leads with is_forced_plan. Closes the previous review's UI finding.
• VERIFIED — "The head is untouched (160-character budget); no test pinned the old wording; Lite has no twin" — changes are all after <<GUIDE>> (:318-320) and the test slices there (DarlingMcpQueryStoreHistoryToolsTests.cs:89); the removed sentences appear nowhere; no McpServerTool outside Darling/ and deprecated/, and neither desktop QueryStoreHistoryWindow renders a forced column.
• UNVERIFIED — the four rows' "waits for CI" — the live row skips without DARLING_TEST_PG (ForcedPlanStateLiveTests.cs:67).
• UNVERIFIED — "builds on macOS … nothing run locally beyond the build" — the author's machine; it needs a log.
• REFUTED — "The wording for the other states is unchanged" — query-store-history.js:148 went from replace("_", " ") to replace(/_/g, " "), so UNKNOWN_PLAN_NOT_COLLECTED now renders unknown plan not collected. A fix, but a wording change to another state, unmentioned.
• REFUTED — "Fixture with two facts for one plan: the fixture now says why" — PickForces_KeepsOneFactPerPlan_AndAStandingForceBeatsARemovedOne still uses two facts for plan 7 in ONE database (DarlingMcpQueryStoreHistoryToolsTests.cs:39-40, both at the db: "Db" default), and :32 does not say the reader cannot emit that. Inline note.

Riskiest lines

1. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpQueryStoreHistoryTools.cs:283 — REMOVED facts now reach forced_plans, where ForceNode renders their unmeasured counters as execution_count: 0 / other_plans_total: 0 (runs are read only for live forces, DarlingForcedPlanStateReader.cs:488-511), beside a plans row with the plan's real executions — UNCOVERED (the live test asserts only plan_id and state, ForcedPlanStateLiveTests.cs:269-271)
2. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpQueryStoreHistoryTools.cs:424 — is_forced_plan must keep its old meaning while forced_state gains a value; a sign error relabels every removed force as standing — covered by IsForced_IsSampledOrStanding_AndARemovedFactAloneIsNeither and TheHistoryTool_ListsAForcedPlanWithNoExecutionsInTheWindow_AndARemovedOneWithItsState
3. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpQueryStoreHistoryTools.cs:403 — the synthesized row for a force with no rows: the standing arm is covered by the same live test (plan 21), the new REMOVED arm is UNCOVERED and inverting IsStanding there fails no test

Traps near this diff

• "A test that constructs a 'four maintenance anomalies in one run' fixture is testing a shape no live run produces (#3709). Fixtures should carry the population the engine actually emits, or say they do not." — applies — DarlingMcpQueryStoreHistoryToolsTests.cs:39-40, a one-database duplicate DarlingForcedPlanStateReader.cs:196-199 cannot emit.
• "Likewise IsDBNull ? 0: an unmeasured pass is null with a *_measured flag beside it, not 0 MB (#3682)." — applies — a REMOVED fact is built from default(Ran) (DarlingForcedPlanStateReader.cs:570-576) and this answer publishes those counters as zeros (DarlingMcpForcedPlanTools.cs:216-220).
• "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 fixed rendering (query-store-history.js:147-149) has no behaviour test; WebQueryStoreHistoryColumnTests.cs:23-41 only pins column flags and the absence of innerHTML.
• "Lite and Darling ship in parity … a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding" — does not apply — no such tool outside Darling/, and Lite/Windows/QueryStoreHistoryWindow.xaml.cs has no forced column.
• "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — does not apply — :403 and :424 route through :294-298.
• "Truncation inferred from a full page: truncated = rows.Count >= limit" — does not apply — plans_truncated is allPlans.Count > MaxPlans (:396); PickForces' Take at :310 sets no flag, a hold the body documents.
• "A JSON number above 2^53 … Emit 64-bit ids as strings" — does not apply — plan_id/query_id are numbers at :416 before and after.

Divergence (blind read vs PR body)

One place, plus a slip.

The body stops at the force's identity and state; the diff also publishes its evidence. forced_plans entries are ForceNodes, carrying forced_plan.execution_count, last_execution_time, other_plans, other_plans_total, variant_plans. For a REMOVED force none of those is measured: runs are read only for forces in the newest capture (DarlingForcedPlanStateReader.cs:488), the fact is built from default(Ran), and the node reads execution_count: 0, last_execution_time: null, other_plans_total: 0. On this PR's own fixture that sits beside plans[0].execution_count == 3 for the same plan 51 — one answer saying the plan ran three times and that the force saw zero runs. get_forced_plans has emitted those zeros since #5632, so it is inherited, not invented; this diff is what puts it in front of the history answer, and the new description invites a reader to trust it. Fix at ForceNode (nulls when State == Removed) or say it in the description. Inline note on :283.

The slip: "the wording for the other states is unchanged" is not what :148 does. An improvement; it belongs in the body.

Everything else matches, including both self-corrections, the standing-first sort, the single IsStanding, and the untouched head.

Reviewer checklist

• Run Darling.Tests with DARLING_TEST_PG set — the live assertion skips silently without it.
• On a real store, read a REMOVED force's whole forced_plans entry and decide whether forced_plan.execution_count: 0 and other_plans_total: 0 are acceptable as published, here and in get_forced_plans.
• Open the web History panel and read the Forced column for all six states, including No (removed) and Yes (unknown plan not collected).

[erikdarlingdata]

Claude posting for Erik Darling.

Pushed 083d627. Answers, one per comment:

• Review, REMOVED evidence as zeros: fixed at ForceNode. A REMOVED force now has runs_read: false and null forced_plan, variant_plans, other_plans, unknown_plans and their totals. get_forced_plans shares the node, so it changes the same way. Both descriptions say null means not measured. New ForceNode_ARemovedForceDoesNotReportZeroRuns_AStandingOneDoes, and the live history test now asserts null runs beside 3 plan executions (waits for CI with DARLING_TEST_PG).
• Review, uncovered synthesized row: the row logic is now WithForcedPlans. WithForcedPlans_AForceWithNoRows_GetsARowThatIsForcedOnlyWhileItStands fails if IsStanding is inverted there.
• Review, wording change: the body and CHANGELOG entry now say UNKNOWN_PLAN_NOT_COLLECTED reads unknown plan not collected.
• Inline, fixture shape: agreed, the reader cannot emit two facts for one plan in one database. The per-plan group and tiebreak were dead code, so I removed them and the doc comment says why. The test now uses facts the reader emits.
• Inline, "standing forces first": moved onto the forced_plans sentence.
• Inline, JS comments: the scope comment now sits above planTable. The wording change is in the body.

Both test projects build with 0 errors on macOS. Nothing ran locally. The PR stays a draft.

Claude posting for Erik Darling. Answered at 083d627: a REMOVED force now reports runs_read false and null run fields instead of zeros (in both tools), the synthesized REMOVED arm and the fixture are fixed, and the body names the wording change. Details in the PR comment.

[claude]

The body says of this line: "No test can fail on it (the grid tests are source pins), so it is unproven until someone looks at the grid." The first half is true of WebQueryStoreHistoryColumnTests, but a behaviour test is available today and needs no new harness plumbing.

Darling/Darling.Tests/web-qs-history-harness.mjs imports the shipped query-store-history.js under Node, renders the panel from a plans fixture (:74-77), and already emits every plan row's rendered text:

out.rowTexts = t.all((n) => n.tag === "tr").slice(1).map((r) => r.textContent);   // :101

QueryStoreHistoryBehaviourTests.Run("open") consumes that scenario and asserts on its DOM output (dataRows, planButtons, chart series). rowTexts is emitted and read by nothing.

So a fixture plan with is_forced_plan: false, forced_state: "REMOVED" plus one assertion that its row text reads No (removed) is a behaviour test that fails on the old forced_state ? "Yes (" + … label and passes on forcedLabel — and a second plan with forced_state: "UNKNOWN_PLAN_NOT_COLLECTED" pins the /_/g arm (unknown plan not collected, where .replace("_", " ") gave unknown plan_not_collected). Neither reads source text, so neither runs into the #5833 ruling.

Both behaviour changes on this line are otherwise uncovered, which is the one thing a reader of the payload cannot check for themselves.

[claude]

The code here is right; the body's description of it is stale. "What changed" still says:

One fact per plan_id (PickForces) … A standing force wins over REMOVED, otherwise the newest LastSeen wins.

Neither mechanism is in this method. There is no LastSeen comparison anywhere in the file, and no per-plan grouping: the standing-before-REMOVED test is an OrderBy ahead of Take(MaxPlans) (:319-320), so it decides which forces survive the cap, not which of two facts for one plan wins. One fact per plan_id is the reader's guarantee, not this method's — DISTINCT ON (database_name, plan_id) plus outer joins grouped the same way (DarlingForcedPlanStateReader.cs:197, :189-206), which is exactly what the new doc comment at :313-315 says and what ToDictionary at :409 depends on.

The Review-round section of the same body already records this correctly ("the per-plan group and its standing-beats-REMOVED tiebreak were dead code. Removed"), so the two halves of the body disagree. Worth deleting the tiebreak sentence from "What changed" and crediting the uniqueness to the read, since that bullet is what a later reader will take as the account of PickForces.

[claude]

Verdict: CHANGES REQUESTED

Claims

• VERIFIED — "Before, a sampled-forced plan showed is_forced_plan: true, forced_state: null, forced_plans: []" — the pre-image filtered State != Removed (DarlingMcpQueryStoreHistoryTools.cs:283 LEFT); old assertion Assert.Empty(...forced_plans) (ForcedPlanStateLiveTests.cs:263-264 LEFT).
• VERIFIED — "REMOVED facts show in forced_plans and as forced_state: REMOVED" — :283, :432, :435; TheHistoryTool_ListsAForcedPlanWithNoExecutionsInTheWindow_AndARemovedOneWithItsState.
• VERIFIED — "Standing forces sort before REMOVED, so REMOVED never pushes a standing force out of the cap" — :319 ahead of Take at :320; PickForces_ListsStandingForcesBeforeRemovedOnes.
• VERIFIED — "is_forced_plan stays forced-when-sampled-or-standing; the synthesized row uses the same IsStanding" — IsForced (:308-309) is value-identical to the old p.IsForcedPlan || forcedById.ContainsKey(...) over a Removed-filtered dictionary; :301 and :431 route through it; IsForced_IsSampledOrStanding_AndARemovedFactAloneIsNeither.
• VERIFIED — "PickForces drops other databases, so ToDictionary cannot throw" — :318 into :409; One("") is All (DatabaseFilter.cs:63-64) and Ordinal matches the read's = ANY on the bare column (:165, never case-folded :23-25), so no row the read returned is dropped.
• VERIFIED — "The read returns one row per database and plan" — DISTINCT ON (database_name, plan_id) (DarlingForcedPlanStateReader.cs:197), joins grouped alike (:189-206), facts 1:1 (:520-578).
• VERIFIED — "A REMOVED force's runs are not read, so the node is runs_read: false with null run fields" — DarlingMcpForcedPlanTools.cs:211-233; exact, since runs are read only for live = forces.Where(InNewest) (DarlingForcedPlanStateReader.cs:488-516) and Classify returns Removed for precisely !inNewestCapture (:344). ForceNode_ARemovedForceDoesNotReportZeroRuns_AStandingOneDoes.
• VERIFIED — "get_forced_plans shares the node; both descriptions say null means not measured" — one ForceNode (:209) called at :180 and :435; sentences at :76 and DarlingMcpQueryStoreHistoryTools.cs:330.
• VERIFIED — "The head is untouched, so the 160-character budget test is unaffected" — every addition follows <<GUIDE>> (:323 vs :329-331; :71 vs :76) and the test measures the pre-marker slice (DarlingMcpQueryStoreHistoryToolsTests.cs:127-133).
• VERIFIED — "No test pinned the old wording" — the deleted sentences appear nowhere else in the tree.
• VERIFIED — "Lite has no twin of this tool" — no such McpServerTool under Lite/Mcp, no Lite forced-plan collector (docs/changelog/3.11.md:161), and QueryStoreHistoryWindow renders the sampled is_forced_plan from DuckDB (LocalDataService.QueryStore.cs:432).
• VERIFIED — "The web panel is the one reader of forced_state" — one non-test occurrence (query-store-history.js:146); the viewer reads is_forced_plan from the store (ViewerDataService.QueryStore.cs:617). It renders no other ForceNode field, so the new nulls cannot reach it.
• VERIFIED — "The regex also fixes UNKNOWN_PLAN_NOT_COLLECTED" — pre-image .replace("_", " ") vs /_/g at :146.
• VERIFIED — "The cap and its lack of a flag are older than this PR" — the pre-image cut at .Take(MaxPlans) with no flag (:283 LEFT).
• UNVERIFIED — "Both test projects build with 0 errors on macOS; nothing ran locally" — the author's machine; needs a log, dotnet is not runnable here.
• UNVERIFIED — the six "waits for CI" revert proofs — the live row skips without DARLING_TEST_PG (ForcedPlanStateLiveTests.cs:67); the unit rows need a run. Each revert is plausible from the code; "fails" is a claim about a run.
• REFUTED — "One fact per plan_id (PickForces) … standing wins over REMOVED, otherwise the newest LastSeen wins" — :317-320 has no LastSeen comparison and no per-plan grouping: standing-first is an ordering ahead of Take, so it decides what survives the cap, not which of two facts for a plan wins; one-fact-per-plan is the read's DISTINCT ON, as :313-315 says. The body's Review round records that group and tiebreak as removed dead code, so "What changed" is stale against the tree and the rest of the body. Inline note.
• REFUTED — "No test can fail on it (the grid tests are source pins)", of forcedLabel — true only of WebQueryStoreHistoryColumnTests: web-qs-history-harness.mjs runs the shipped query-store-history.js under Node on a plans fixture (:74-77) and already emits each rendered row's text (:101), consumed by QueryStoreHistoryBehaviourTests.Run("open"). A fixture plan with is_forced_plan: false, forced_state: "REMOVED" plus one assertion that its row reads No (removed) fails on the old label and passes on forcedLabel; a second with UNKNOWN_PLAN_NOT_COLLECTED pins the /_/g arm. Neither reads source text. Inline note.

Riskiest lines

1. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpForcedPlanTools.cs:211 — read = p.State != Removed gates seven published fields on BOTH tools and must be exactly the reader's live predicate — covered by ForceNode_ARemovedForceDoesNotReportZeroRuns_AStandingOneDoes
2. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpQueryStoreHistoryTools.cs:318 — the database equality decides which facts reach forced_plans and keeps ToDictionary at :409 single-keyed — covered by PickForces_KeepsOnlyTheNamedDatabase
3. Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/query-store-history.js:147 — the Forced cell's text for every state, changed two ways at once — UNCOVERED, and the harness that would cover it already emits the row text (web-qs-history-harness.mjs:101)

Traps near this diff

• "an unmeasured pass is null with a *_measured flag beside it, not 0 MB (#3682)" — does not apply — this diff is the fix: a REMOVED fact's runs come from default(Ran) (DarlingForcedPlanStateReader.cs:570-576) and are now null beside runs_read: false, on get_forced_plans too.
• "Ruling (#5833): tests are load-bearing … A trap is answered with a behaviour test that fails on the defect and passes on the fix, or with one sentence in the PR body saying why none is needed." — applies — query-store-history.js:147 has neither: only source pins (WebQueryStoreHistoryColumnTests.cs:23-41), and the sentence offered is untrue of web-qs-history-harness.mjs:101.
• "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 — no counterpart exists (docs/changelog/3.11.md:161; nothing under Lite/Mcp).
• "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — does not apply — :301 and :431 both call IsStanding/IsForced (:305-309).
• "A total_* assigned from a .Count is a count of the page unless the set is provably whole (#3699)" — does not apply — the *_total count the reader's whole lists; Take(MaxOtherPlans) cuts only what is listed (DarlingMcpForcedPlanTools.cs:212-233).
• "Truncation inferred from a full page: truncated = rows.Count >= limit" — does not apply — plans_truncated is allPlans.Count > MaxPlans (:407); PickForces' Take sets no flag, pre-existing (:283 LEFT) and held deliberately.
• "A JSON number above 2^53 is rounded by every JavaScript client" — does not apply — plan_id/query_id are numbers at :423 and DarlingMcpForcedPlanTools.cs:217-218, before and after.
• "Every MCP filter is part of the query, not a post-filter on the page (#3641)" — does not apply — PickForces post-filters a read already narrowed on database and query_id (DarlingForcedPlanStateReader.cs:185-187), so nothing is cut after a LIMIT.

Divergence (blind read vs PR body)

Two places, both in the body rather than the code. The diff read clean: the read flag is exactly the reader's live predicate, is_forced_plan is value-preserving for every plan with history rows, the standing-first sort protects a standing force from the cap, and the new nulls cannot reach the only UI that consumes this payload.

1. "What changed" credits PickForces with a mechanism that is gone. The LastSeen tiebreak and the per-plan group were removed as dead code — the body's own Review round says so — yet the bullet still describes them and still attributes one-fact-per-plan to PickForces instead of the read's DISTINCT ON.
2. The one uncovered behaviour change is declared untestable, and it is not. forcedLabel changes two things at once and the body closes it with "No test can fail on it". The Node harness that renders this panel already publishes the rendered row text and nothing reads it.

Reviewer checklist

• Run Darling.Tests with DARLING_TEST_PG set — the live arm skips silently without it (ForcedPlanStateLiveTests.cs:67).
• Open the web History panel on a query whose force was removed in the window and read the Forced column for each reachable state: No (removed), Yes (removed) for a plan also sampled as forced, Yes (unknown plan not collected).
• On a real store, read one REMOVED entry of get_forced_plans end to end and confirm an agent reads runs_read: false plus the nulls as "not measured" — a payload change for every client of that tool, not only the history read.

[erikdarlingdata]

Claude posting for Erik Darling.

Answers to the second review at d60d268:

• Review (forced label untested): added TheForcedColumn_ReadsARemovedForceAsNoRemoved_AndSpacesEveryUnderscoreInAStateName. A new forcedLabels harness scenario renders two plans and the test reads rowTexts. The Node run printed No (removed) and No (unknown plan not collected). The old label reads Yes (REMOVED) and unknown plan_not_collected, so both assertions fail on it. The open fixture is unchanged.
• Inline, query-store-history.js claim "no test can fail on it": removed from the body, the new test is in the tests table.
• Inline, PickForces body text: rewritten. The reader's DISTINCT ON gives one fact per plan, and PickForces only filters by database and sorts standing before REMOVED ahead of the cap.

Claude posting for Erik Darling. Answered at d60d268: a behaviour test reads the grid's rendered Forced label for a REMOVED force and an underscored state, and the body now credits one fact per plan to the reader's DISTINCT ON. Details in the PR comment.

[claude]

Non-blocking: this is a second spelling of DarlingMcpQueryStoreHistoryTools.IsStanding (DarlingMcpQueryStoreHistoryTools.cs:305), both added in this PR, and nothing holds the two equal.

They agree today for a reason that lives in a third file: Classify returns Removed for exactly !inNewestCapture (DarlingForcedPlanStateReader.cs:344), and runs are read only for live = forces.Where(InNewest) (:488-511), so State != Removed is "runs were read". That chain is correct but implicit. If a future state ever means "the force ended" without meaning "runs were not read" (or the reverse), runs_read goes true while own/others/variants are still default(Ran) and the fabricated zeros this PR just removed come back on both tools at once — with no test failing, because every current test exercises Removed.

Either call DarlingMcpQueryStoreHistoryTools.IsStanding(p) here so there is one predicate, or say in the summary that read tracks the reader's live set rather than "standing" and that the two happen to coincide. The summary at :206-207 names the mechanism, which is most of the way there; it is the shared spelling that is duplicated.

[claude]

Non-blocking, fixture shape. The second plan pairs is_forced_plan: false with forced_state: "UNKNOWN_PLAN_NOT_COLLECTED", which the tool cannot emit: is_forced_plan = IsForced(p.IsForcedPlan, fact) is true for every standing state (DarlingMcpQueryStoreHistoryTools.cs:431, IsStanding at :305), so forced_state non-null with is_forced_plan false is REMOVED and only REMOVED. That row renders No (unknown plan not collected) — a label the page can never show; the reachable one is Yes (unknown plan not collected).

It does not weaken the test: the assertion is Assert.Contains("unknown plan not collected", rows[1]), which fails on the old .replace("_", " ") either way. But REVIEW-TRAPS' #3709 line asks fixtures to carry the population the engine actually emits, and is_forced_plan: true here costs nothing and makes the second row the shape a real answer has — with the first row still carrying the No (removed) arm, which is the one that needs is_forced_plan: false.

[claude]

Claude review, full pass at d60d268. Both findings from the pass at 083d627 are answered in the tree, not just in the body:

• forcedLabel coverage. QueryStoreHistoryBehaviourTests.cs:91-98 runs the shipped query-store-history.js under Node through the new forcedLabels scenario (web-qs-history-harness.mjs:115-122) and reads the rendered <tr> text. It fails on the old label on both arms: "Yes (removed)" for row 0 and "unknown plan_not_collected" for row 1. Not a source pin.
• The PickForces body text. "What changed" now attributes one-fact-per-plan to the read's DISTINCT ON and credits PickForces with only the database filter and the standing-first sort, which is what :317-320 does.

One observation that is older than this PR and out of its scope, recorded so it is not re-discovered:

query-store-history.js:211 writes "Only the " + d.plans.length + " plans with the most total duration are listed." under plans_truncated. d.plans is the post-WithForcedPlans list, so when the cap bites it carries the 20 longest plus every forced plan added back past it — the count is not 20, and the rows added back are specifically not among the longest. This PR can widen that gap (a REMOVED force with no runs in the window now gets a synthesized zero row too), but the mechanism predates it: the pre-image added forced plans past the cap the same way.

Same scope note for the forced_plans cap itself — PickForces' .Take(MaxPlans) sets no flag, so a query with more than 20 forces drops the rest silently against the description's "Every plan of the query forced in its database's newest forced-plan capture is listed". The body calls this out and declines it deliberately, and I agree with declining it here: the pre-image cut at the same Take, and the standing-first sort means this change cannot newly displace a standing force. If it is ever worth a flag, it wants its own change with a name for the cut.

[claude]

Verdict: LGTM

Claims

• VERIFIED — "Before, a plan forced when sampled showed is_forced_plan: true, forced_state: null, forced_plans: []" — the pre-image filtered State != Removed (DarlingMcpQueryStoreHistoryTools.cs:283 LEFT); old assertions Assert.Empty(...forced_plans) and null forced_state (ForcedPlanStateLiveTests.cs:263-264 LEFT).
• VERIFIED — "REMOVED facts show in forced_plans and as forced_state: REMOVED" — :283 into :432/:435; TheHistoryTool_ListsAForcedPlanWithNoExecutionsInTheWindow_AndARemovedOneWithItsState.
• VERIFIED — "One fact per plan comes from the reader's DISTINCT ON (database_name, plan_id); PickForces does no per-plan choice" — DarlingForcedPlanStateReader.cs:197, facts built 1:1 (:520-578); :317-320 has no grouping and no tiebreak.
• VERIFIED — "PickForces keeps the named database only (a blank name widens the read to every database)" — :318; One("") is All (DatabaseFilter.cs:63-64) and the read never trims or case-folds (:23-25), so no returned row is dropped; it also keeps ToDictionary at :409 single-keyed. PickForces_KeepsOnlyTheNamedDatabase.
• VERIFIED — "Standing sorts before REMOVED, so REMOVED never pushes a standing force out of the 20 cap" — :319 ahead of Take at :320; PickForces_ListsStandingForcesBeforeRemovedOnes.
• VERIFIED — "is_forced_plan stays forced-when-sampled-or-standing; the synthesized row uses the same IsStanding" — IsForced (:308-309) is value-identical to the old p.IsForcedPlan || forcedById.ContainsKey(...) over a Removed-filtered dictionary, so no plan's flag moves; :301 and :431 route through it. IsForced_IsSampledOrStanding_..., WithForcedPlans_AForceWithNoRows_....
• VERIFIED — "A REMOVED force's runs are not read, so runs_read: false and null run fields" — DarlingMcpForcedPlanTools.cs:211-233; exact, since ran is filled only for live = forces.Where(InNewest) (DarlingForcedPlanStateReader.cs:488-511) and Classify returns Removed for precisely !inNewestCapture (:344), so those fields were default(Ran) and empty lists. ForceNode_ARemovedForceDoesNotReportZeroRuns_AStandingOneDoes.
• VERIFIED — "get_forced_plans shares the node; both descriptions say null means not measured" — one ForceNode (:209) called at :180 and :435; sentences at :76 and :330. McpHelpers.JsonOptions sets no DefaultIgnoreCondition (McpHelpers.cs:63), so the nulls are written as keys, not dropped.
• VERIFIED — "The web panel is the one reader of forced_state; it reads No (removed)" — one shipped occurrence (query-store-history.js:146); both desktop windows bind a plain IsForcedPlan checkbox off the stats flag with no force state (Lite/Windows/QueryStoreHistoryWindow.xaml:135, Viewer :149), and get_forced_plans reaches the page only through the name list in plain-text.js:30, which consumes no structure.
• VERIFIED — "The head is untouched, so the 160-character budget test is unaffected; no test pinned the old wording" — every addition follows <<GUIDE>> (:323 vs :329-331; DarlingMcpForcedPlanTools.cs:71 vs :76), the test measures the pre-marker slice (DarlingMcpQueryStoreHistoryToolsTests.cs:127-133), both tails stay far under the 24,000 cap (McpToolGuide.cs:51), and the deleted sentences appear nowhere else.
• VERIFIED — "Lite has no twin; no migration rung, no new tool" — neither tool is declared under Lite/Mcp, ForcedPlanState is Darling.Storage only (DarlingForcedPlanStateReader.cs:20), and the diff touches no PgMigrations rung.
• VERIFIED — "The regex also fixes UNKNOWN_PLAN_NOT_COLLECTED" — the pre-image's string-pattern .replace("_", " ") takes the first underscore only; /_/g at :146 takes all.
• VERIFIED — "The cap and its lack of a flag are older than this PR" — the pre-image cut at the same Take(MaxPlans) with no flag (:283 LEFT).
• UNVERIFIED — the six "waits for CI" revert proofs — each revert is plausible from the code, but "fails it" is a claim about a run; the live row skips silently without DARLING_TEST_PG (ForcedPlanStateLiveTests.cs:67).
• UNVERIFIED — "Darling.Tests builds on macOS" / the forcedLabels Node run printing both labels — the author's machine; dotnet is not runnable here, so even compilation of the new conditionals is unchecked.
• UNVERIFIED — "20 forces on one query has no known trigger" — a claim about live stores, not readable from the tree.

Riskiest lines

1. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpForcedPlanTools.cs:211 — read gates seven published fields on BOTH tools and must stay exactly the reader's live predicate or the fabricated zeros return — covered by ForceNode_ARemovedForceDoesNotReportZeroRuns_AStandingOneDoes
2. Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpQueryStoreHistoryTools.cs:319 — the standing-first sort is the whole reason admitting REMOVED facts cannot cost a standing force its slot in the cap — covered by PickForces_ListsStandingForcesBeforeRemovedOnes
3. Darling/PerformanceMonitor.Darling.Service/wwwroot/js/pages/query-store-history.js:147 — the Forced cell's text for every state, changed two ways at once — covered by TheForcedColumn_ReadsARemovedForceAsNoRemoved_AndSpacesEveryUnderscoreInAStateName, which renders the shipped file under Node and reads the row text

Traps near this diff

• "Likewise IsDBNull ? 0: an unmeasured pass is null with a *_measured flag beside it, not 0 MB (#3682)." — applies, and this diff is the fix — a REMOVED fact's runs came from default(Ran) and empty lists (DarlingForcedPlanStateReader.cs:570-577) and published as measured zeros; now null beside runs_read: false, on get_forced_plans too.
• "Ruling (#5833): tests are load-bearing. … A trap is answered with a behaviour test that fails on the defect and passes on the fix, or with one sentence in the PR body saying why none is needed." — applies and is satisfied — all seven new tests are behaviour tests, and query-store-history.js:147, uncovered at 083d627, now has QueryStoreHistoryBehaviourTests.cs:91-98 over the rendered rows (web-qs-history-harness.mjs:121).
• "Lite and Darling ship in parity (CONTRIBUTING's two-store rule): a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding, not a nit" — does not apply — no counterpart exists; the two desktop Forced columns read the sampled flag only (Lite/Windows/QueryStoreHistoryWindow.xaml:135).
• "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — applies weakly, non-blocking — :301 and :431 call the new IsStanding/IsForced, but DarlingMcpForcedPlanTools.cs:211 spells the same predicate again; they agree today only through Classify (DarlingForcedPlanStateReader.cs:344). Inline note.
• "A total_* assigned from a .Count is a count of the page unless the set is provably whole (#3699)" — does not apply — the three *_total count the reader's whole lists; only the emitted arrays are cut, at Take(MaxOtherPlans) (DarlingMcpForcedPlanTools.cs:212-233).
• "Truncation inferred from a full page: truncated = rows.Count >= limit" — does not apply — plans_truncated is allPlans.Count > MaxPlans over the whole fold (:407), strictly greater and not off a page.
• "Every MCP filter is part of the query, not a post-filter on the page (#3641)" — does not apply — PickForces re-filters a read already narrowed on database and query_id inside the statement, ahead of the MaxForces cap (DarlingForcedPlanStateReader.cs:185-187, :208).
• "A JSON number above 2^53 is rounded by every JavaScript client (#3487)" — does not apply — plan_id/query_id are Query Store ids, emitted as numbers before and after (:423, DarlingMcpForcedPlanTools.cs:217-218), unchanged by this diff.

Divergence (blind read vs PR body)

None — the body's account matches the diff on every claim graded above. Both divergences the pass at 083d627 reported are gone: "What changed" no longer credits PickForces with a LastSeen tiebreak or a per-plan group (:317-320 has neither), and forcedLabel is no longer declared untestable. My blind read reached the body's own reading on the part that mattered most: IsForced is value-preserving, so admitting REMOVED facts to forcedById adds forced_state without moving any plan's is_forced_plan.

Reviewer checklist

• Run Darling.Tests with DARLING_TEST_PG set — the live arm skips silently without it (ForcedPlanStateLiveTests.cs:67) and is the only test driving the REMOVED path end to end through real SQL.
• Confirm the build: dotnet is not runnable here, so the new read ? new { … } : null conditionals over anonymous types in ForceNode and the seven new tests are unverified as compiling.
• Open the web History panel on a query whose force was removed in the window and read the Forced column for each reachable state — No (removed), Yes (removed) for a plan also sampled as forced, Yes (working). The Node harness drives a stand-in DOM, not a browser.
• Read one REMOVED entry of get_forced_plans on a real store: this is a payload change for every client of that tool, not only the history read, and runs_read: false plus seven nulls has to read as "not measured" to an agent that previously saw zeros.