Finish #456's descent: score, override, display, and share proc-body statements - #486
Merged
Merged
Conversation
#456 taught the parser and analyzer to descend into stored procedure and UDF bodies via the shared PlanStatements.EnumerateAll, but four consumers kept walking batch.Statements and quietly saw a different plan: - BenefitScorer.ScoreCancellable never visited body statements, so their warnings had no MaxBenefitPercent and no wait-stat scoring — the UI sorts unquantified warnings last, burying every finding of an EXEC plan. - PlanAnalyzer.ApplySeverityOverrides skipped body statements, so a user's severity override silently did not apply inside a procedure. (MarkLegacyWarnings was checked and is fine: it runs per-statement from inside the analyzer's EnumerateAll loop.) - The desktop statements grid and the MCP session registration showed one row (the EXEC's synthetic root) and near-zero counts while Human/Robot Advice discussed warnings the UI could not display or navigate to. The same mismatch lived in McpQueryStoreTools.CaptureSession, whose counts now come from the analysis summary stored on the same session, and in ParsedPlan.AllMissingIndexes, which feeds both registrations. - The web viewer's statement tabs index result.Statements (EnumerateAll order) but ActiveStmtPlan mapped that index into the outer-only batch list, so clicking a body statement's tab rendered no operator tree. Body statements need a visible home, so the traversal grew a context- carrying form, EnumerateAllWithContainer, that pairs each statement with the module path it lives in ("dbo.Proc", "dbo.Outer > dbo.Inner"); EnumerateAll is defined on top of it so the two cannot diverge. The grid prefixes body rows with that path for display only — copy and open-in-editor still hand out the statement text exactly as recorded. Tests: body warnings carry benefit scores (fixture) and wait-stat warnings (synthetic nested plan); severity overrides reach body statements; container paths are asserted; and a headless-UI test drives the real PlanViewerControl over the exec_stored_procedure fixture, asserting the grid row count matches EnumerateAll, body rows carry the module prefix, and selecting a body statement renders its operators. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
#431 made AnalysisJson the one place that knows how deep a serialized AnalysisResult goes (MaxDepth 1024 against the System.Text.Json default of 64, two JSON levels per operator), precisely because inline options keep getting rebuilt without it. Review found three more such sites, all on the web share path, so a deep-but-realistic plan (~30 nested operators) analyzed fine and then failed to Share — or shared and failed to load — with an "object cycle" message pointing at the wrong cause: - PlanShareService.ShareAsync serialized the upload envelope with default options; - PlanShareService.LoadAsync parsed the share with default JsonDocumentOptions and deserialized the result at the default 64 again; - server/PlanShare's /api/share parsed the uploaded body with default JsonDocumentOptions, turning a legitimate deep upload into 400 "Invalid JSON" before ttl_days was ever read. AnalysisJson grows a Wire options set (default formatting, only the ceiling raised — shares already in the database were written unindented with nulls, and the fix is the ceiling, not a wire-format change) and a Document counterpart for JsonDocument.Parse call sites. The web project links AnalysisJson.cs the way it links the rest of Core's sources. The server cannot reference PlanViewer.Core, so it mirrors the constant as a literal with a comment naming AnalysisJson as the source of truth — a shared constant only helps call sites that reference it; this one cannot. The Core depth test now says so too, as the tripwire for anyone changing the number. Tests: the exact share envelope shape ({result, text, ttl_days} → JsonDocument → GetRawText → Deserialize) round-trips a 100-operator chain through the shared options, and the default reader is shown to reject the same payload so the options are provably load-bearing. The Web and server call sites themselves are out of this suite's reach (Blazor WASM project not referenced; server references nothing), so they are verified by inspection and the contract is pinned here. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n
|
Reviewed the diff in full (server/PlanShare, PlanViewer.App control/MCP code, PlanViewer.Core traversal/scoring/JSON options, PlanViewer.Web share path, and the new/changed tests). This is a clean, well-targeted fix — no correctness, injection, or crash issues found. Specifically verified:
One non-issue worth flagging as an FYI rather than a defect: in |
This was referenced Sep 3, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes the four analysis-integration findings from the 2026-09-02 adversarial review — #456 taught the parser and analyzer to descend into procedure/UDF bodies, but four consumers never followed:
batch.Statementsonly: body-statement warnings got noMaxBenefitPercentand no wait-stat scoring, so the UI's benefit sort quietly buried every finding inside anEXECplan. Now walksPlanStatements.EnumerateAll.MarkLegacyWarningswas verified already-covered: it rides inside the analyzer's own descending loop.)dbo.Proc > SELECT …) via a newEnumerateAllWithContainertraversal (defined so the plain and context-carrying walks are literally one walk), and selecting a body statement renders its operators — verified on the exec-proc fixture in the headless harness. Adjacent mirrors fixed in-pass: MCP counts come from the stored analysis summary,AllMissingIndexesdescends, and the web viewer's statement-tab index now maps throughEnumerateAll(clicking a body statement's tab used to render no tree).AnalysisJsongainsWire(deliberately not the WithoutNulls variants — existing DB shares stay readable) andDocumentoptions; the server mirrors the constant with a comment naming the source of truth, since it deliberately takes no Core reference.How was this tested?
Six new tests (proc-body scoring/overrides, grid contents via the headless harness, and a share-envelope round-trip of a 100-operator chain that the default reader provably rejects). Full suite at dev tip + fix: 414 tests, 413 passed, 1 platform skip, 0 failed, on Windows.
Deploy note:
server/PlanShare/Program.cschanged — PlanShare deploys by hand (scp + systemctl on the Hetzner box), and a redeploy was already owed for the range's dependency bump.🤖 Generated with Claude Code
https://claude.ai/code/session_01PvAv72Pwb8czsjDWsCCk7n