[erikdarlingdata]

Closes #1914

The headline: #1882 shipped guidance that fails when followed, and this fixes it

#1882 told operators how to scope a force-plan recommendation to a replica:

"pass it as the fourth argument (they are extended stored procedure arguments, so the documented order matters — see the reference page)"

That came from the reference page's syntax block and had never been run. It fails.

sp_query_store_force_plan is an EXTENDED_STORED_PROCEDURE, so sys.system_parameters returns zero rows for it — its argument surface cannot be read, only executed. #1914 executed it, on SQL Server 2022 (16.0.4255.1) and SQL Server 2025 (17.0.4045.5), each form run from a freshly-unforced plan in a throwaway database.

Call form	2022	2025
@query_id, @plan_id	OK	OK
@query_id, @plan_id, @replica_group_id = 1	OK	OK
@query_id, @plan_id, @disable_optimized_plan_forcing = 0, @replica_group_id = 1 (the documented order)	ERROR 12463	ERROR 12463
same, with @disable_optimized_plan_forcing = 1	OK	OK
@plan_id before @query_id	OK	OK
@replica_group_id = 99	ERROR 12463	OK ⚠
@replica_groupid = 2 (misspelled)	reached replica logic	reached replica logic
@there_is_no_such_parameter = 1	OK, silently	OK, silently

Error 12463 is "Role id should be between (including) 1 and 4" — for a role id of 1, which is in that range. The form the reference page documents is the one that errors; the form #1882 declined to emit is the one that works. And named arguments can be reversed, so the doc's "documented order matters" warning does not describe named-argument behavior at all.

The rendered guidance now names the three-argument form and says explicitly not to add @disable_optimized_plan_forcing to reach it.

Two findings that changed what the text tells an operator to do

SQL Server 2025 does not validate the group id. @replica_group_id = 99 — not a role at all — succeeded on a standalone server and wrote a row into sys.query_store_plan_forcing_locations naming replica group 99. SQL Server 2022 rejects the same call. So on the version where the feature is GA, a wrong id does not fail loudly; it records a forcing against a replica that does not exist. The text now says to check the id against sys.query_store_replicas first.

Nothing protects a typo. A misspelled @replica_groupid still reached the replica-group logic, and an entirely invented parameter name was accepted with no error at all.

Also observed, and consistent with the settled Azure facts: sys.query_store_replicas holds 0 rows on a standalone 2022 but a static 4-row roles enumeration on a standalone 2025 (Primary / Secondary / Geo Secondary / Geo HA Secondary) — on a box with no Availability Group. Same shape already recorded for Azure SQL DB.

Why the issue's own proposal is closed rather than built

#1914 asked whether to collect replica_group_id so the recommendation could emit a scoped call instead of describing one. The probes answer no:

1. The value would have to be persisted, and sys.query_store_replicas accumulates a row per (replica, role) across failovers — the docs' own remark — so a persisted id can silently re-point at a different replica.
2. On 2025 a wrong id does not error. A stale id would not fail; it would force against a replica that does not exist.
3. The id is not even needed to be correct to be accepted, so we would be shipping a computed value into a call that cannot check it.

An operator reading the current ids off the live server at the moment they act is the only reliable version of this, and the guidance now tells them to do exactly that.

No collector column, no storage migration, no StorageVersion bump, no Darling migration number claimed. (The store-shape instructions in the assignment are therefore moot — nothing was added.)

Watched-red

Reverted the guidance text to its pre-#1914 wording: 2 of 21 failed — TheScopedFormItSuggests_IsTheOneThatActuallyWorks and ItTellsTheOperatorToVerifyTheGroupId_BecauseSql2025DoesNotValidateIt. The other 19 are #1882's behavioral pins, which must hold unchanged.

One pre-existing assertion needed tightening rather than deleting: Assert.DoesNotContain("@replica_group_id =", sql) pinned "we never emit a scoped call" as a text absence over the whole blob, which broke the moment the disclosure showed the operator the form in a comment. It now asserts over executable lines only (non---), which is the invariant that actually matters — nothing SQL Server would run carries a replica group id.

Testing

-t:Rebuild clean: 0 errors, 0 warnings.

Suite	Result
Dashboard.Tests	768 passed, 0 failed
Darling.Tests	3818 passed, 0 failed, 214 skipped
Lite.Tests	1897 passed, 0 failed

6,483 passing. Live SQL Server 2022 and 2025 used for the probes; both throwaway databases dropped and verified GONE on both instances. No Azure provisioning — the questions were box-SQL-Server questions, and the Azure half was already settled in the validation-runs reference (static 4-row roles enumeration, all stats replica_group_id = 1, replica_role constant Primary).

Deferrals

None. Nothing downscoped; no new issue needed. The proposal is answered on evidence rather than deferred, and the shipped-guidance bug it uncovered is fixed here rather than filed.

[claude]

Review

What this does: Corrects the force-plan replica-scoping guidance emitted by AppendSecondaryReplicaDisclosure (in PerformanceMonitor.Analysis/FactRemediation.cs, the preview path behind FactRemediation.GenerateForFinding). #1882 told operators to pass @replica_group_id "as the fourth argument," which #1914's live probing on SQL Server 2022 and 2025 showed actually errors (12463); the three-argument named form is the one that works. The PR also adds two new findings to the guidance text: SQL Server 2025 accepts an out-of-range @replica_group_id without erroring (silently recording a forcing against a nonexistent replica), and typos/unknown parameter names are accepted silently. New tests pin the corrected wording and tighten a prior DoesNotContain assertion to executable lines only. No schema/storage changes -- text-only fix, consistent with the PR description.

The reasoning and live-probe evidence in the PR description and doc comments are solid, and the corrected guidance for the preview path is thorough.

Parity/correctness gap (not caught by CI)

FactRemediation.cs has two independent places that render this replica-scoping disclosure:

1. AppendSecondaryReplicaDisclosure (~line 874) -- feeds GenerateForFinding, the preview text. Fully corrected by this PR.
2. The inline disclosure inside RenderCopyPasteCommand's PLAN_REGRESSION branch (~lines 1304-1306) -- not touched by this PR, still reads:
   ```csharp
   var disclosure = IsNonPrimaryReplicaRow(t.ReplicaRole)
   ? $"-- Measured on the {t.ReplicaRole} replica; this forces on the PRIMARY (@replica_group_id" + nl +
   "-- defaults there when omitted). Scope it with @replica_group_id to target that replica." + nl
   : string.Empty;
   ```

RenderCopyPasteCommand is the single shared renderer both apps delegate to for their copy-paste and MCP surfaces -- Lite/Mcp/McpAnalysisTools.cs, Darling/PerformanceMonitor.Darling.Service/Mcp/DarlingMcpTools.cs, Darling/PerformanceMonitor.Darling.Viewer/RecommendationsViewModel.cs, and Lite/Analysis/Recommendations/LiteRecommendationsReader.cs all call it. It still tells the operator to "Scope it with @replica_group_id" with no warning against adding @disable_optimized_plan_forcing (the thing that produces error 12463) and no mention of the SQL Server 2025 silent-acceptance gap that this PR's own description calls out as changing "what the text tells an operator to do."

This is arguably the higher-stakes surface to leave stale, not the lower one: the MCP tools hand this string to an LLM/agent as `remediation_command` with no human reading a "preview" first. The existing test for this exact block (`Lite.Tests/ForcePlanReplicaScopeTests.cs:369`, `CopyPasteSql_SecondaryTarget_CarriesTheDisclosureWithTheStatement`) states the principle this now violates in its own comment: "a warning that only exists in the preview is a warning the person running the statement never sees." That test only asserts the old generic text is present, so nothing in CI catches the drift between the two disclosures.

Suggest bringing the copy-paste disclosure in sync -- even kept compact, e.g. adding "Do not add `@disable_optimized_plan_forcing` (error 12463 on both versions). Check the id against `sys.query_store_replicas` first -- 2025 accepts an invalid id without error."

(Since `RenderCopyPasteCommand` is the genuinely shared source for both apps, this isn't a Lite-vs-Darling drift -- it's a drift between two renderers within the shared library that both apps now inherit identically. Flagging it here because the PR's stated goal is specifically "guidance that fails when followed," and this second surface still ships the pre-fix framing.)

Everything else

• No Lite/Darling drift otherwise found -- `FactRemediation.GenerateForFinding` is exclusively Lite's live-preview path (Darling reads persisted findings and uses `RenderCopyPasteCommand`), and that split is intentional/pre-existing, not something this PR needs to reconcile.
• Test changes look correct: the `DoesNotContain` tightening to executable-lines-only is a real improvement (a bare whole-blob check couldn't distinguish "we emit the scoped call" from "we show it in a comment").
• No security concerns -- all interpolated values (`QueryId`, `PlanId`) are typed as `long`, and `Database` goes through `QuoteName`.
• CHANGELOG entry is consistent with the code change.