analyze_query_plan moves reading guidance off tools/list (#3898)

[erikdarlingdata]

Refs #3898.

Why

analyze_query_plan is a twin tool served on both products. Both descriptions were over the ~800-char
threshold this wave converts (Darling 915 served chars, Lite 903). Splitting them moves reading guidance off
tools/list into a per-tool guide that get_tool_guide serves on demand. Nothing is deleted.

What changes

• Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpPlanTools.cs and Lite/Mcp/McpPlanTools.cs:
  analyze_query_plan's head is now the shared plan-tool guardrail block, ruled final and shared verbatim with
  analyze_plan_xml (analyze_plan_xml moves reading guidance off tools/list (#3898) #4115, merged) and analyze_procedure_plan (analyze_procedure_plan moves reading guidance off tools/list (#3898) #4117). This replaces the lane's own earlier
  head wording. The <<GUIDE>> marker and the entire original tail are unchanged on both products. Only the
  text before the marker moved.
• Darling/Darling.Tests/McpToolGuideHeads.AnalyzeQueryPlan.cs and Lite.Tests/McpToolGuideHeads.AnalyzeQueryPlan.cs:
  rewritten to a HeadFacts array, the same pattern analyze_procedure_plan's own head-pin files use. It pins
  the new head's guardrail facts instead of the old ones.
• Budget: the analyze_query_plan line only, 613 -> 616 (the corrected served length), in both
  Darling/Darling.Tests/McpToolsListBudget/DarlingMcpPlanTools.txt and Lite.Tests/McpToolsListBudget/McpPlanTools.txt.
  No other tool's line in either file changed.
• Lite.Tests/McpDescriptionTruthPinTests.cs and Darling/Darling.Tests/DarlingMcpPlanToolsTests.cs: not
  touched by this PR. Both files' existing plan-tool pins pass unmodified against the new head. Every fragment
  they check is verbatim in the new shared block. That covers the impact_basis label, the create_statement
  sentence, the corroboration sentence, the fixed caveat, and the regression-risk clause. See "Coordinator
  verification" below.
• The changelog buffer entry (4126-reported.txt, outside the repo) corrected: 613 -> 616.

Served-chars table

Tool	Darling before	Darling after	Lite before	Lite after
analyze_query_plan	915	616	903	616

The head is byte-identical on both products: 585 chars, served 616 with the 31-char pointer. It was built from
one set of string literals applied to both files. It was then confirmed with desc.py analyze_query_plan, and
with the generic McpToolGuideTests.EverySharedToolName_CarriesTheMarkerOnBothSkus_OrNeither_WithByteIdenticalHeads
test in Darling.Tests, which scans both SKUs' source. Both passed.

Corrections

None found in the tail. The original prose was checked against AnalyzeQueryPlan on both products,
McpPlanAnalysisFormatter.BuildAnalysisResult, DarlingEngineCapability.NotCollectedStatusAsync, and
CollectorEngineCapability.NotCollectedMessage. All of it held up. This pass re-checked the "latest plan"
ORDER BY and the not_collected/unavailable route order against current code for the new head. See the file:line
citations under "Coordinator verification."

D4 removals

None. The original sentences on each product carry no issue references and no anecdotes, so nothing needed to
come off the wire. The tail is the full original text, unchanged by this pass. Only the head text changed.
Dropcheck confirms this: missing=0 on both products (see Test plan).

Coordinator verification

This PR was at the D9 finish-pass stage. The shared plan-tool guardrail ruling applies here, and a prior edit
to the shared test helper needed to be reverted. That work is done.

• The ruling was applied. analyze_query_plan's head is now the same 419-char guardrail block, verbatim
  (including the U+2014 dash), that analyze_plan_xml and analyze_procedure_plan carry. The block was copied
  from analyze_procedure_plan's already-ruled head, not re-derived, and split across source lines the same
  way: one concatenated string per sentence.
• The helper edit was reverted. The coordinator already restored Lite.Tests/McpDescriptionTruthPinTests.cs to
  dev's version before this pass started. This pass does not touch it, or its Darling twin
  DarlingMcpPlanToolsTests.cs. Both files' existing plan-tool pins pass unmodified (see Test plan).

New head (585 chars, served 616, identical on both products):

Analyzes query_hash's latest plan. No plan: not_collected if the engine can't collect query_stats, else
unavailable. Per statement: warnings; missing_indexes labelled impact_basis, with create_statement -- the
optimizer's suggested CREATE INDEX for this statement: corroboration for a statement already measured slow,
never a diagnosis; every row carries the fixed caveat (regression risk for other plans, write cost);
parameters; memory_grant; top_operators by operators_ranked_by, with operators_returned / total_operators /
truncated. actual_* are null, not 0, without runtime stats.

Every head fact checked against the code, file:line on each product:

• "Analyzes query_hash's latest plan." Both products fetch the plan ordered collection_time DESC LIMIT 1.
  Lite: Lite/Services/LocalDataService.QueryStats.cs:667. Darling:
  Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingStoredPlanReader.cs:70, the
  QueryStatsPlanXmlByHashSql constant, read from GetQueryStatsPlanXmlByHashAsync at line 122.
• "No plan: not_collected if the engine can't collect query_stats, else unavailable." Both route
  NotCollectedStatusAsync first. unavailable is only the fallback when that call returns null.
  Lite: Lite/Mcp/McpPlanTools.cs:28-32 calling Lite/Mcp/McpEngineCapability.cs:43-69. Darling:
  Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpPlanTools.cs:59-63 calling
  Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingEngineCapability.cs:62-82. Both return the
  not_collected envelope only when the collector cannot run on the server's probed engine. A registry read
  that fails answers null on both products, falling through to the ordinary unavailable miss, not a third
  status.
• The shared guardrail block itself: the impact_basis label, the create_statement corroboration and
  never-a-diagnosis caveat, and the fixed per-row caveat naming regression risk and write cost. Copied verbatim
  from analyze_procedure_plan's ruled head, not re-derived for this tool.
• "actual_ are null, not 0, without runtime stats."* One shared formatter serves both products:
  PerformanceMonitor.PlanAnalysis/McpPlanAnalysisFormatter.cs:129-131. actual_rows, actual_elapsed_ms,
  and actual_cpu_ms are each (long?)null when HasActualStats is false, never 0.
• No "LATEST IS A TIME" catalog phrase. analyze_query_plan analyzes one stored plan by query_hash. It
  is not a latest-snapshot read across many rows in wave E lesson 5's sense, so the phrase does not apply.

PR tender verification

• Head facts fixed: 0. Each head fact holds on both products:
  • "latest plan": both reads take the newest captured plan, ORDER BY collection_time DESC LIMIT 1. See
    Lite/Services/LocalDataService.QueryStats.cs:658 (GetCachedQueryPlanAsync) and
    Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingStoredPlanReader.cs:70-71.
  • "No plan: not_collected ... else unavailable": NotCollectedStatusAsync runs first, then unavailable.
    See Lite/Mcp/McpPlanTools.cs:35-39 and DarlingMcpPlanTools.cs:66-70. An exception returns error
    through McpHelpers.FormatError.
  • "actual_* are null, not 0, without runtime stats": actual_rows, actual_elapsed_ms and actual_cpu_ms
    are null when the plan has no runtime stats. See
    PerformanceMonitor.PlanAnalysis/McpPlanAnalysisFormatter.cs:129-131.
• Merge: 20abc27d merged dev after analyze_procedure_plan moves reading guidance off tools/list (#3898) #4117 landed, since analyze_procedure_plan moves reading guidance off tools/list (#3898) #4117 changed the same plan files. The merge was clean.
  Both budget lines stay on both products: analyze_procedure_plan 618 and analyze_query_plan 616.
• D9 round 1 used haiku and 8 questions: the 6 traps below, one on Darling's database_name, and one on
  whether the plan is read live. It found 0 misreads. Q4, Q6 and Q8 were answered "can't tell". The other five answers were
  correct.
• Targeted tests after the merge: Lite 118 total, 0 failed. Darling 343 total, 0 failed, 9 skipped. The classes
  are the Test plan's list plus *McpPlanAnalysis*.
• Changelog buffer: 4126-reported.txt is true as written (915 and 903 served characters before, 616 after).

D9 traps

• Q: A top_operators row has no actual_elapsed_ms. Does that mean the operator ran in 0 ms?
  Correct: No. actual_* fields go null, not 0, when the plan carries no runtime statistics. A 0 there
  is a real measurement.
  Prevents it: "actual_* are null, not 0, without runtime stats."
• Q: The tool answers status: "unavailable". Does that mean the same thing as status: "not_collected"?
  Correct: No. not_collected means the engine itself cannot collect query_stats, checked first.
  unavailable means the engine can collect it, but there is no stored plan for this one hash.
  Prevents it: "No plan: not_collected if the engine can't collect query_stats, else unavailable."
• Q: create_statement returns a ready-made CREATE INDEX statement. Is it safe to run as returned?
  Correct: No. It is corroboration for a statement already measured slow, never a diagnosis on its own.
  Every row carries the fixed caveat: regression risk for other plans, and write cost.
  Prevents it: "corroboration for a statement already measured slow, never a diagnosis; every row carries
  the fixed caveat (regression risk for other plans, write cost)."
• Q: Calling analyze_query_plan twice in a row for the same query_hash is guaranteed to return the same
  plan both times.
  Correct: No. It always analyzes query_hash's latest plan, so a newer capture landing between the two
  calls changes the answer.
  Prevents it: "Analyzes query_hash's latest plan."
• Q: One response shows missing_indexes rows from two statements with impact_basis 40 and 30. Does
  combining the index help by 70 percent?
  Correct: No. The block opens "Per statement," and that scopes everything under it to its own statement:
  warnings, missing_indexes/impact_basis, parameters, memory_grant, top_operators. The two numbers are not
  from the same scope, so they are not additive.
  Prevents it: "Per statement: warnings; missing_indexes labelled impact_basis, ..."
• Q: A response's warnings array is empty. Does that mean the query has no performance problems?
  Correct: No. The head promises only five categories per statement: warnings, missing_indexes, parameters,
  memory_grant, top_operators. An empty array in one category means that category had nothing to report, not
  that the query is problem-free overall.
  Prevents it: "Per statement: warnings; missing_indexes labelled impact_basis, ...; parameters;
  memory_grant; top_operators by operators_ranked_by, ..."

Test plan

• Build Darling/Darling.Tests/Darling.Tests.csproj: Build succeeded, 0 Warning(s), 0 Error(s).
• Build Lite.Tests/Lite.Tests.csproj: Build succeeded, 0 Warning(s), 0 Error(s).
• Targeted Darling classes: McpToolGuide*, McpToolsListBudget*, McpDescription*,
  McpZeroIsAMeasurement*, McpPayloadContractCensus*, MeasurementContractCensus*, McpPageContract*,
  PlanTools*, AnalyzeQueryPlan*. Total 315, Failed 0, Skipped 9 (live-Postgres classes, no rig in this
  pass).
• Targeted Lite classes, same filters. Total 101, Failed 0, Skipped 0.
• dropcheck.py origin/dev HEAD analyze_query_plan, run against the pushed commit. Darling:
  old=915 new=1511 sentences=3 missing=0. Lite: old=903 new=1499 sentences=3 missing=0.
• desc.py analyze_query_plan: head 585, served 616 on both products.
• Plain-English checker on this PR body: run once this pass, real hits fixed.
• Full Darling/Lite suites: not run in this finish pass. The brief for this pass scopes targeted classes
  only, and says not to run a full suite. CI runs the full suites on the push. This pass changes 6 files: 2
  tool source, 2 head-pin tests, 2 budget lines, all covered by the targeted classes above.

Handoff

• Nothing is left unconverted in this lane's scope. analyze_query_plan is fully converted on both products,
  with the ruled shared head.
• analyze_plan_xml (analyze_plan_xml moves reading guidance off tools/list (#3898) #4115) is merged into dev. analyze_procedure_plan (analyze_procedure_plan moves reading guidance off tools/list (#3898) #4117) has since merged into dev as aed89b93.
  This pass only read its worktree as the reference for the block's exact wording and line-split style. It did
  not modify that PR.
• analyze_query_store_plan stays unconverted, per the wave-E common brief.
• A full-suite flake noted earlier in this PR's history is AnalysisPassTokenThreadingTests on Lite, unrelated
  to MCP descriptions. It was not re-verified in this pass, since the full suite did not run here. It is worth
  a look if it recurs in a CI census.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NUU29PuGg9TUFBsACgZg2K