Fix CLI exit code and --output handling, and stop the web viewer crashing on plans with no statements

[erikdarlingdata]

What does this PR do?

This PR fixes three review findings, F4, F2, and F3. Each fix has its own commit. It also fixes a fourth problem of the same kind, in a separate commit.

F4: the web page crashed on a plan with no statements

The web page read the first statement of the result without checking that one exists. Some XML parses without an error and still has no statement. Well formed XML that is not a showplan is one case. A showplan with an empty batch is another. That input got past the page's error handling. The result view then threw a NullReferenceException while it drew.

The page now shows "Could not parse any statements from the plan XML" in the box that it uses for its other load errors. It checks after parsing. It checks again after it loads a shared plan, because a stored result with no statements fails the same way.

The decision is one Core method, PlanStatements.NoStatementsMessage. It uses the same statement walk as the result mapper. It returns a message exactly when the mapped result has no statement. The Blazor page cannot be referenced from PlanViewer.Core.Tests. For that reason the decision lives in Core, in a file that the Web project already links, and the tests pin it there. The page only calls it.

I checked the desktop app and the CLI with the same input. planview analyze <file> already refused it. The desktop app already handles it, with different text: "The plan parsed but contains no statements to display." That code is in PlanViewerControl, which another PR owns, so I left it alone. The web page uses the CLI's text. The MCP and REPL tools use the same words, with a final period.

The CLI had a gap. analyze --server and query-store only checked for a parse error. For a plan with no statements they wrote an empty analysis and reported OK. PlanAnalysisRunner.ParseFailure now also returns the no-statements message, and all three CLI paths call it. The single-file path lost its own copy of the check. Its message and exit code did not change.

F2: query-store exited 0 when a plan failed

When one plan failed, the command printed the error and wrote an ERROR row in summary.txt. The process still exited 0, so a script had no way to tell that the run was incomplete.

The loop over the fetched plans now counts the plans that failed. The command sets exit code 1 when the count is above zero. That is the rule analyze --server already follows, and Program.cs already returns the value. Plans that worked keep their files and their summary rows. When it fetched more than one plan, the command also prints Processed N plans: X succeeded, Y failed, as analyze does for files.

The loop moved into QueryStoreCommand.AnalyzePlansAsync. It takes the fetched plans and a log writer, so a test can run it without a SQL Server. No command documents its exit codes, so I did not add documentation for this one.

F3: an unknown --output value did nothing

An --output value other than json, text, or both got through. query-store and analyze --server wrote no files for it and exited 0. analyze <file> printed json.

PlanAnalysisRunner.CreateOutputOption now builds the option with a check on its value. The two commands that declare --output, analyze and query-store, both use it. A bad value is now a parse error. The message names the allowed values, the exit code is 1, and the command does no work. The help line shows <both|json|text>.

WriteResultFilesAsync also throws an ArgumentException for an unknown format. It throws before it writes a file and before it trims the operator trees. A caller that skips the option cannot get an empty run that looks fine. In query-store, that exception counts as a failed plan, so the exit code is 1.

Two behavior notes:

• Any letter case is accepted, as it is for --order-by. -o JSON on a single file printed json before, and it still does. Both commands read the value in lowercase through ReadOutputFormat.
• Without --server, analyze --output both still prints json. The help text says so. The README now says that both writes files with --server.

Program.cs built the command tree inline. It now calls a new CliRoot.Create. The tests walk that same tree and check every command that declares --output, so a command added later is checked too.

Also fixed: an unknown --order-by value in query-store

I found this while fixing F3. It is the same problem. The query lowercases the value and falls back to CPU order for one it does not know. A misspelled metric ran to the end ranked by CPU, and summary.txt still said "top by" the misspelling.

The option now checks the value while the command line is parsed. It ignores letter case, as the query does and as --auth does. It names the allowed values and exits 1. The help text did not change. It is now built from the same list that the check uses. This is a separate commit, so it can be dropped on its own.

Which component(s) does this affect?

• Desktop App (PlanViewer.App)
• Core Library (PlanViewer.Core)
• CLI Tool (PlanViewer.Cli)
• SSMS Extension (PlanViewer.Ssms)
• Tests
• Documentation

The web viewer (PlanViewer.Web) is not in the template list. This PR changes it too. The documentation change is one line in the README CLI reference.

How was this tested?

I ran everything on Windows. No SQL Server was involved. The tests use the fixture row_goal_plan.sqlplan, and small inline XML for the inputs that have no statements.

• Full suite on c30613e: total 1155, failed 0, succeeded 1125, skipped 30. The skipped tests are platform specific (the macOS keychain and non-Windows Entra).
• dotnet build PlanViewer.sln -c Debug --no-incremental and dotnet build src/PlanViewer.Web -c Release --no-incremental: 0 warnings and 0 errors.

This PR adds 28 tests in four classes. Where I say that a test fails without the fix, I removed the fix, ran the class, and put the fix back. The child-process tests use a small helper, CliProcess. It runs the built planview.dll the same way HistoricalCliContractTests does.

NoStatementsGuardTests (11 tests) covers F4.

• Three inputs with no statements parse with no ParseError. They are non-showplan XML, a showplan with an empty batch, and a showplan with no batch sequence. The mapped result has no statements. The Core decision returns the message.
• The steps that the page runs before it draws (analyze, score, map, format) do not throw for such a plan. That shows why only the new check stops it.
• A real plan is not refused. A stored result with no statements is.
• ParseFailure returns the message for all three inputs. planview analyze <file> on non-showplan XML exits 1 with the same message and prints nothing on stdout.
• Without the ParseFailure change, 4 of these 11 tests fail. The tests for the web page decision do not compile without the new method.

QueryStoreAnalyzePlansTests (2 tests) covers F2.

• The four plans are good, malformed XML, non-showplan XML, and good. The failure count is 2. Both good plans have their .sqlplan, .analysis.json, and .analysis.txt files, including the plan after the failures. The failed plans have no analysis files. summary.txt has four rows, and two of them are ERROR rows.
• With only good plans, the count is 0.
• Without the failed++ line, the first test fails (expected 2, actual 0).

OutputOptionTests (13 tests) covers F3.

• A walk over the tree in CliRoot finds every command that declares --output. Each one refuses bogus after --output and after -o, with an error that names bogus and all three allowed values. Each one accepts json, text, and both, in any letter case.
• Two child-process tests run planview analyze <file> --output bogus and planview query-store ... --output bogus. Both exit nonzero, name the allowed values on stderr, and do no work. A third runs planview analyze <file> -o TEXT and checks that it prints exactly what -o text prints.
• The file writer writes only the files that each of the three values names. It refuses bogus, JSON, and an empty string, with nothing written and the result unchanged.
• If an unknown format reaches AnalyzePlansAsync, every plan counts as failed.
• Without AcceptOnlyFromAmong, 3 tests fail. Without the writer's check, 4 tests fail.

OrderByOptionTests (3 tests) covers the --order-by fix.

• An unknown value is a parse error that names the allowed values. Each listed value is accepted in lower case, upper case, and capitalized form. The default is still cpu.
• Without the check, the first test fails.

Not done

• The Razor lines that show the message have not run in a browser. The build passes with no warnings, and the tests cover the decision that the lines call.
• The line in QueryStoreCommand.RunAsync that sets the exit code has no test, because RunAsync needs a live SQL Server. The tests cover the count that it uses.
• The desktop app still uses its own text for the no-statements case. See F4.
• query-store still exits 0 when Query Store has no data for the time range. No plan failed, so I left it.
• If the share server returns a result of null, the page still shows the empty landing view with no message. It does not crash, and it is not about statements.

Checklist

• I have read the contributing guide
• My code builds with zero warnings (dotnet build -c Debug)
• All tests pass (dotnet test)
• I have not introduced any hardcoded credentials or server names

🤖 Generated with Claude Code

https://claude.ai/code/session_019n3G844aTidqrD6A6iMgza

[claude]

Reviewed the CLI --output/--order-by validation, the no-statements guard, the query-store exit code, and the Web changes. I found no blocking issues.

• PlanStatements.cs and the Output/* files are already linked in PlanViewer.Web.csproj, so the new NoStatementsMessage overloads compile in Blazor.
• No version bump, no new NoWarn, and no SQL generation in this diff.
• Minor: ParseFailure now covers the empty-plan case, but I only skimmed the tests. Please confirm one exercises the live and Query Store paths with well-formed non-showplan XML, since that's the behavior change.
• Minor: QueryStoreCommand now exits 1 on any failed plan. That is a behavior change for scripts, so it's worth a line in the release notes.

[erikdarlingdata]

Both notes checked. QueryStoreAnalyzePlansTests runs well-formed non-showplan XML through the Query Store loop: it counts as a failed plan and writes no analysis files. The live path calls the same ParseFailure, which the tests call directly for all three no-statement inputs. Running the live path end to end needs a SQL Server, so it has no child-process test. The exit-code change for query-store goes in the release notes.