Put the parameter values back where the analysis is built (#482)

[erikdarlingdata]

Closes #482. Out of #467, out of #466.

What was wrong

@joshdbe confirmed #467 worked and then said the rest of the app hadn't moved:

Shows actual values in Copy/Paste and query editor. Still shows parametrized in Human and Robot Advice

He's right, and it's the same mistake as #447. #467 fixed the three consumers I
happened to be looking at — Copy Query Text, Open in Query Editor, and Ctrl+C
on the statements grid — and every other consumer went on handing out @0.

There aren't two bugs here, there's one seam. Advice for Humans is
TextFormatter.Format(analysis). Advice for Robots is
JsonSerializer.Serialize(analysis). Both read StatementResult.StatementText,
which comes from ResultMapper.MapStatement, and so does the HTML export, the
comparison report, PlanOperations' ranked-operator and warning labels, and the
MCP tools. Fixing one place fixes all of them, and the MCP tools are the ones
that mattered most: handing a model @0 and no values is handing it a question
it cannot answer.

The parser's PlanStatement is deliberately untouched. The analyzer's rules
regex over that text — OPTIMIZE FOR UNKNOWN, NOT IN, the MAXDOP 2 hint,
the row-goal cause — and the properties panel is a view of the XML. Neither
should start reading manufactured literals. WarningBaseline.txt did not move,
which is the evidence that the change landed at the right layer rather than a
comment claiming it did.

The question the issue told me to answer first

Does anything match, pair, key or dedupe on statement text? No.
ComparisonFormatter.MatchStatements pairs on QueryHash, then falls back to
position; statement text is only ever printed, truncated to 500 characters.
Nothing else in the repo groups, keys or hashes on it. So the failure mode I was
warned about — two runs of one query with different parameter values no longer
matching each other — does not exist here. There is a test that pins it anyway,
because "it pairs on QueryHash" is a fact about today's code and this is exactly
the sort of thing that gets quietly rewritten.

Is the CLI's JSON a contract? Yes, and the repo says so in its own words.
HistoricalCliContractTests hashes analyze --compact against a pinned SHA256,
and the comment on that constant says it has rolled twice, both times additively,
"so a consumer reading fields by name is unaffected — but anything diffing or
hashing whole output sees different bytes, which is exactly what this constant
exists to make somebody decide on rather than discover."

That alone would have been arguable. What settled it is a consumer inside this
repo that genuinely needs the parameterized form: get_repro_script falls back
to StatementText and hands it to ReproScriptBuilder, which wraps the body in
sp_executesql with a parameter list read out of the same plan. A body with the
literals already inlined would declare parameters that appear nowhere in it, and
the plan it produced would be the constant-folded one rather than the
parameterized compile the repro script exists to reproduce. That is precisely
the "something was quietly relying on it" case, and it is not hypothetical.

So: both forms

StatementText is the runnable text. parameterized_statement_text carries the
plan's own record, and is null unless something was actually substituted —
which is nearly every statement, so the JSON does not grow for plans this does
not touch. get_repro_script reaches for it and falls through to StatementText,
which is the same string whenever there was nothing to substitute.

The pinned CLI hash is still green, and not by luck: row_goal_plan.sqlplan has
no ParameterList, so its statement_text is byte-identical and the new key is
absent. A test pins both halves of that.

I did consider the other shape — leave statement_text alone and add
runnable_statement_text — which would have been purely additive. I didn't take
it, for the reason this issue exists: it would leave statement_text, the
obvious field, still saying @0, and every future consumer would default to the
wrong one. That is how #467 and #447 both shipped incomplete. The right default
belongs at the seam.

One thing the issue didn't ask for, which this change forced

compile_memory_exceeded_plan.sqlplan is
SELECT @job_name = name, @owner_sid = owner_sid FROM msdb.dbo.sysjobs_view WHERE (job_id = @job_id),
and its ParameterList records both assigned variables with a compiled value of
NULL. Substituted blindly that reads SELECT NULL = name, NULL = owner_sid —
not merely unrunnable but quietly misleading, because it now looks like a
comparison.

That defect is #467's, not this PR's. But it was reachable only by explicitly
choosing "Copy Query Text (with values)", and this change makes it what the
advice, the exports and the MCP tools show by default. Shipping that knowingly
isn't on, so ParameterSubstitution grew an assignment-target check: a
parameter followed by a lone = and preceded by SELECT, SET, or a list
comma is being assigned to, not read from, and keeps its name. @job_id in the
WHERE still gets its value.

It is three lead-ins and a forward scan, not a parser, and it is deliberately
narrow. A parameter on the right of an = is a read. A parameter followed by
>=, <=, <> or != is a comparison, because the scan forward meets that
operator's own character rather than the =. SELECT VoteTypeId = @VoteTypeId
— an alias on the left, the parameter on the right, which is the shape
param-sniffing-posttypeid2 carries — is still substituted. All four of those
have tests, and the two guarding against over-suppression were proven red
against an over-eager rule rather than against no rule at all, since no rule
passes them trivially.

Also: the web viewer links Core files by hand

PlanViewer.Web.csproj lists the Core sources it compiles one by one, so
ResultMapper reaching for ParameterSubstitution broke the Blazor build while
the full test suite stayed green. The file is now on the list. It is pure string
work with no WASM-hostile dependencies, and the web viewer renders the same
AnalysisResult, so it gets the fix too.

What this does not do

• It does not touch the plan properties panel. Text shows what the plan
  records, next to ParameterizedText, and that panel stays a view of the XML.
• It does not touch the analyzer. Every rule still reads the plan's own text.
• It does not widen ReproScriptBuilder's parameter-name check. @0 … @6
  fail its ^@[\p{L}_@#$] identifier test and get dropped, so a
  forced-parameterization plan never reaches its sp_executesql branch at all.
  That's arguably wrong — sp_executesql N'…', N'@0 varchar(8000)', @0='123456'
  is legal — but it is a separate question from this one and I left it alone.
• get_plan_parameters now shows the substituted statement next to the
  parameter list it is describing, so the @0 anchor is gone from that label.
  The values are right there in the same object, so it reads fine, but it is a
  deliberate call rather than an oversight.

Tests

Thirteen new, in two files: the analysis-level consumers in
AnalysisParameterSubstitutionTests, the substitution rule itself alongside
#467's own cases in ParameterSubstitutionTests.

Every one was proven red first, against four separate reverts, because a test
that passes either way is how #467 shipped incomplete:

• Revert the mapper → 7 of the 9 analysis tests fail. The two that don't are
  the preservation pins, which is the point of them.
• Remove the assignment guard → the 3 assignment tests fail.
• Make the assignment guard over-eager → the 2 over-suppression guards fail,
  and so does one of Offer to put parameter values back when copying a statement out of a plan #467's existing tests, which is a decent sign the wrong
  rule really is wrong.
• Implement Advice, exports and MCP still hand out @0 instead of the values #482 the naive way (substitute in place, no second form, repro
  script reading StatementText) → the 2 preservation pins fail. Those are red
  against exactly the tempting implementation, which is the only counterfactual
  that makes them worth having.

Suite: 392 on origin/dev (018a82f), 405 here, 0 failed, 2 skipped
(Windows-only), four consecutive full runs, ~20s each, no flakes.
WarningBaseline.txt unchanged. Full solution builds clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_016a1AnKAHwcALrwdYVVrpgR

[claude]

IsAssignmentTarget only recognizes plain = as an assignment operator, so T-SQL's compound assignment forms (+=, -=, *=, /=, %=, &=, ^=, |=) fall through as reads and get overwritten with their own value — the same failure mode this function exists to prevent.

Trace: SET @a += 1 with @a's compiled value 5. At @a, forward lands on + (not =), so the function returns false at line 143-144 before ever reaching the lead-in check. Apply then substitutes, producing SET 5 += 1 — broken text that would show up in Advice for Humans/Robots, the HTML export, and the comparison report (though not get_repro_script, which prefers ParameterizedStatementText).

Plans that capture a bare SET @var += ... as a StmtSimple (e.g. inside a procedure body) will hit this. Worth at least checking text[forward] is '=' or '+' or '-' or '*' or '/' or '%' or '&' or '^' or '|' followed by = (and not ==, which doesn't exist in T-SQL anyway) so compound assignments are recognized too.

[claude]

Reviewed. The seam analysis in the PR description checks out — ResultMapper.MapStatement is genuinely the single point every consumer (Advice for Humans/Robots, HTML export, comparison report, PlanOperations ranked-operator/warning labels, MCP tools) reads through, and the parser-layer PlanStatement used by the analyzer's regex rules and the properties panel is correctly left untouched. ComparisonFormatter.MatchStatements does key on QueryHash with position fallback, not statement text, so the two-runs-different-text case is safe as claimed. get_repro_script correctly reverts to preferring ParameterizedStatementText so the declared sp_executesql parameter list still matches the body. PlanViewer.Web.csproj picked up the new linked <Compile Include> for ParameterSubstitution.cs, consistent with the repo's convention for Core files the Blazor app needs. Values substituted into HTML output go through HttpUtility.HtmlEncode, and JSON output goes through System.Text.Json, so untrusted plan-XML content isn't an injection concern for the display paths this touches.

One finding, left inline: IsAssignmentTarget only checks for a bare =, so compound assignment (SET @a += 1) isn't recognized as a target and gets overwritten with its own value — the same bug class this PR fixes for plain =, just not covered for the compound forms. Narrow (plans rarely capture bare compound-assignment StmtSimple nodes), but a straightforward one-line extension of the existing check if it's worth closing.

No other correctness, security, or convention issues found. Test coverage for the new behavior is thorough — the thirteen new tests hit assignment/non-assignment shapes, the four-consumer fan-out, the QueryHash-pairing safety property, and the CLI-contract-preserving no-op case.