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

[erikdarlingdata]

Refs #3898.

Why

analyze_procedure_plan served 926 characters on Darling and 912 on Lite in every tools/list call. Most of that text is reading guidance: how to interpret impact_basis, why create_statement is corroboration and not a diagnosis. It also covers how top_operators is capped and ranked. A model needs that guidance once it already has a result in hand, not before it decides whether to call the tool. This PR moves that guidance to get_tool_guide, the same way earlier #3898 lanes converted the HealthParser and Data families.

What changes

• Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpPlanTools.cs: analyze_procedure_plan's Description now carries a terse head, the <<GUIDE>> marker, then the full original text as the tail.
• Lite/Mcp/McpPlanTools.cs: the same split, with Lite's own first sentence (it keys the lookup by plan_handle, not sql_handle) in its own tail.
• New pin files: Darling/Darling.Tests/McpToolGuideHeads.AnalyzeProcedurePlan.cs, Lite.Tests/McpToolGuideHeads.AnalyzeProcedurePlan.cs.
• Budget lines for analyze_procedure_plan in Darling/Darling.Tests/McpToolsListBudget/DarlingMcpPlanTools.txt and Lite.Tests/McpToolsListBudget/McpPlanTools.txt.
• No parameter, SQL, payload, or behavior change anywhere. Descriptions only.

Before/after served characters

Tool	Darling before	Darling after	Lite before	Lite after
analyze_procedure_plan	926	618	912	618

Both products now serve the identical 618-character head, which satisfies the D6 lockstep rule. The two products key the lookup by a different parameter: sql_handle on Darling, plan_handle on Lite. The head states both in one clause rather than picking one product's word. D6 allows this when the underlying fact itself differs by product.

D6 note: the tool body, not just the description

Darling reads a stored plan out of its own Postgres store (DarlingStoredPlanReader.GetProcedurePlanXmlBySqlHandleAsync, keyed by sql_handle, the procedure_stats.sql_handle hex string). Lite reads a cached plan out of its local DuckDB v_query_stats view (LocalDataService.GetCachedProcedurePlanAsync, keyed by plan_handle). This parameter difference is real, not a naming inconsistency to paper over, so the head names both explicitly. Neither product's identifier works against the other product's tool.

Status routes (every status word the head states)

Status	Product	File : line	Condition
not_collected	Darling	DarlingMcpPlanTools.cs:97	GetProcedurePlanXmlBySqlHandleAsync returned null/empty AND this server's probed engine cannot run procedure_stats collection at all
unavailable	Darling	DarlingMcpPlanTools.cs:98	Same null/empty read, but the engine CAN collect procedure_stats: no stored plan exists for this sql_handle, not captured or aged out of the store
error	Darling	DarlingMcpPlanTools.cs:106	Any exception during resolve/read/build (McpHelpers.FormatError, status error on both products)
not_collected	Lite	McpPlanTools.cs:66	GetCachedProcedurePlanAsync returned null/empty AND the engine cannot run procedure_stats collection
unavailable	Lite	McpPlanTools.cs:67	Same null/empty read, engine-capable: no stored plan for this plan_handle, never captured or evicted from the plan cache since the last collection
error	Lite	McpPlanTools.cs:75	Any exception (McpHelpers.FormatError)

not_collected is checked before unavailable on both products. The capability probe runs first on every miss. So an unavailable answer is never proof the engine lacks this capability. It only means this specific handle has no stored plan right now.

Corrections

None. The original text's behavioral claims all matched the tool body. This includes the key parameter names, create_statement described as corroboration, impact_basis described as a per-statement estimate, and the operator ranking switch. Each also matched the published constants in PerformanceMonitor.PlanAnalysis/McpPlanAnalysisFormatter.cs verbatim.

D4 removals

None. The original description carried no issue references and no anecdotes.

Re-pointed pins

None in the final version. The lane first changed the shared Description() helper in Lite.Tests/McpDescriptionTruthPinTests.cs to read head plus tail. The coordinator pass reverted that, so this PR no longer touches the file. The helper reads the served head on purpose (#3898 D3). A model needs these caveats from tools/list before it acts on a missing-index row.

The head now carries every fragment that PlanTools_DescribeTheStatedOperatorCut_AndTheCreateStatement pins, in 587 characters: labelled impact_basis, 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, operators_ranked_by and operators_returned / total_operators / truncated. #4115 also makes the pin's DoesNotContain checks read head plus tail. That covers the lane's concern about the #3696 suppression sentence coming back in the tail.

D9 traps

• Q: analyze_procedure_plan returns status unavailable. Does that mean the plan collector is broken?
  Correct: Not always. A missing plan can mean the handle was never captured, or it aged out of the store.
  Prevents it: "No plan: not_collected if the engine can't collect procedure_stats, else unavailable."
• Q: The tool returns status not_collected. Is a stopped collector the cause?
  Correct: No. not_collected means this engine cannot produce procedure_stats plans at all. A capable engine's stopped collector is a different case, and it reports unavailable instead.
  Prevents it: "No plan: not_collected if the engine can't collect procedure_stats, else unavailable."
• Q: unavailable and not_collected both mean no plan came back. Are they the same signal for deciding whether to escalate?
  Correct: No. not_collected leaves nothing to escalate, because this engine never produces one. unavailable means the engine can produce one, and this one is missing right now. That is worth checking against the collector.
  Prevents it: "not_collected if the engine can't collect procedure_stats, else unavailable" gives each status its own condition.
• Q: Which parameter do I pass to identify the procedure's plan?
  Correct: It depends on the product. Darling takes sql_handle. Lite takes plan_handle. No single name works on both.
  Prevents it: "by sql_handle (Darling) or plan_handle (Lite)"
• Q: I have a handle string from Darling's get_top_procedures_by_cpu. Does it work if I pass it to Lite's analyze_procedure_plan, since both tools share a name and parameter shape?
  Correct: No. Each product's get_top_procedures_by_cpu returns the identifier in its own product's shape. The two are not interchangeable, even though the tool name matches.
  Prevents it: "by sql_handle (Darling) or plan_handle (Lite)". Each product's parameter text names get_top_procedures_by_cpu as the source.
• Q: missing_indexes[].create_statement gives runnable CREATE INDEX DDL. Is that reason enough to run it?
  Correct: No. It is corroboration for a statement already measured slow, not a standalone diagnosis. It also carries write cost and regression risk for other plans.
  Prevents it: "corroboration for a statement already measured slow, never a diagnosis"
• Q: missing_indexes[].impact_basis is a percent. Does a bigger number mean more total server-wide benefit if I add several suggested indexes?
  Correct: No. It is one statement's own estimated cost reduction, taken alone. It does not add up across statements or rows, and it does not measure server load.
  Prevents it: "missing_indexes labelled impact_basis". Each row's impact_basis string says the estimate is statement-scoped and not additive.
• Q: Two missing_indexes rows on the same statement carry different impact_basis values. Does the higher one alone tell me which index to build first?
  Correct: No. Neither row is a diagnosis by itself. Both are corroboration for a statement independently measured slow, so the choice needs other evidence, not just a bigger percent.
  Prevents it: "corroboration for a statement already measured slow, never a diagnosis"
• Q: top_operators lists some rows. Is that the complete operator tree for the statement?
  Correct: No. It is a fixed cut of the 10 most expensive operators per statement. The rest of the plan is not in this list.
  Prevents it: "with operators_returned / total_operators / truncated"
• Q: Does top_operators rank the same way on every call, so two payloads' top rows compare directly?
  Correct: No. It ranks by measured actual_elapsed_ms when the plan has runtime statistics. Without runtime statistics, it ranks by the optimizer's cost_percent estimate instead. One payload's ranking can be a measurement, and another's a guess.
  Prevents it: "top_operators by operators_ranked_by". Each payload states its own ranking basis.
• Q: The head lists warnings, missing indexes, parameters, memory grants, and top_operators together. Does the fixed 10-row cut apply to the warnings list too?
  Correct: No. The cut and its ranking basis are stated for top_operators alone. warnings is a separate list with no cap.
  Prevents it: "top_operators by operators_ranked_by, with operators_returned / total_operators / truncated" names the cut on top_operators alone. Nothing in the head caps warnings.
• Q: Is analyze_procedure_plan a live query against the monitored server, so the result reflects what the server is doing right now?
  Correct: No. It analyzes a stored plan. Darling reads its own Postgres store, and Lite reads its local DuckDB cache. Neither is a live fetch.
  Prevents it: "Analyzes a procedure's stored plan"

Test plan

• Build Darling.Tests: 0 Warning(s), 0 Error(s).
• Build Lite.Tests: 0 Warning(s), 0 Error(s).
• Targeted (Darling): ran McpToolGuide*, McpToolsListBudget*, McpDescription*, McpZeroIsAMeasurement*, McpPayloadContractCensus*, DarlingMcpPlanTools*, DarlingWebEndpoints*. Total 295, 0 failed.
• Targeted (Lite): ran McpToolGuide*, McpToolsListBudget*, McpDescription*, McpZeroIsAMeasurement*, McpPayloadContractCensus*. Total 59, 0 failed after the re-point above. Before that re-point, 1 failed.
• Red-watch: removed "not_collected: procedure_stats unsupported on this engine." from the head. Confirmed Head_CarriesEveryGuardrailFact_AndStaysUnderTheCap failed with "Not found". Restored the text and confirmed green again.
• dropcheck.py origin/dev HEAD analyze_procedure_plan: missing=0 on both Darling and Lite.
• git merge origin/dev (clean, no conflicts).
• Full suite, Darling.Tests: 13,337 total, 0 failed, 636 skipped, 1 not run.
• Full suite, Lite.Tests: 5,270 total, 1 failed, 0 skipped. The one failure is AnalysisPassTokenThreadingTests.TheReadLockWaitIsAbandonableWhileAWriterHoldsIt, a lock-abandon concurrency test. It has no connection to this PR's files (MCP plan-tool descriptions and one test helper). It passed cleanly when re-run alone. That makes it a timing flake in the full-suite run, not a regression from this change.
• Plain-English checker on this PR body: ran once, fixed the real hits, did not re-run to chase a clean score.

Coordinator verification

• I merged origin/dev (90d6af4a) into the branch. The dropped-sentence check shows missing=0 on both products. No parameter description changed.
• I checked the lane's head against the code. Darling reads DarlingStoredPlanReader by sql_handle. Lite reads v_query_stats in its DuckDB store by plan_handle, newest capture first. On a miss, both call NotCollectedStatusAsync for procedure_stats first and return unavailable only when it returns null. Every head fact was correct. Head facts fixed: 0.
• D9 round 1, on the lane's head: 12 questions, 0 misreads. Q8, Q10 and Q11 came back as "can't tell".
• Head rewrite: the lane's head dropped the pinned fragments and re-pointed the pin instead (see "Re-pointed pins"). The new head keeps the lane's key and status clauses and carries every pinned fragment. It uses the same wording as analyze_plan_xml's head (analyze_plan_xml moves reading guidance off tools/list (#3898) #4115), so the plan tools describe the missing-index fields in one voice. The served size went from 608 to 618 on both products. I updated both head-pin files and both budget lines.
• D9 round 2, on the new head: 12 questions, 0 misreads. Q2, Q4, Q7, Q8, Q10 and Q11 came back as "can't tell". Q7 lost its answer because the head no longer says impact_basis is not additive. The head names the label, and each row's impact_basis string says "statement-scoped, not additive".
• After the edits, both builds have 0 warnings. Targeted classes: Darling 339 tests (8 skipped) and Lite 99 tests, 0 failed. CI runs the full suites before the merge.
• The changelog buffer entry says "fewer than 620 characters", which is still correct.

Handoff

Nothing deferred. The one tool in this lane's budget is fully converted, green, and pinned on both products.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NUU29PuGg9TUFBsACgZg2K