Release v1.20.0 - #445
Merged
Merged
Conversation
Every Dependabot PR carried a red X from the review check, which failed
twice over instead of reviewing: claude-code-action rejects non-human
actors outright ("Workflow initiated by non-human actor: dependabot"),
and a dependabot-triggered run draws from the Dependabot secret store,
not the Actions one, so CLAUDE_CODE_OAUTH_TOKEN was empty regardless.
Both open bumps (#420, #422) build and pass tests; only this check
blocked them. A version bump has nothing an AI review would catch that
build-and-test doesn't, so skip - neutral, not red - in the same spirit
as the existing draft/fork/release-PR exclusions.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dabot Skip the AI review job on Dependabot PRs
Bumps ModelContextProtocol from 1.4.1 to 2.0.0 Bumps ModelContextProtocol.AspNetCore from 1.4.1 to 2.0.0 --- updated-dependencies: - dependency-name: ModelContextProtocol dependency-version: 2.0.0 dependency-type: direct:production update-type: version-update:semver-major - dependency-name: ModelContextProtocol.AspNetCore dependency-version: 2.0.0 dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
….bundle_e_sqlite3 Bumps Microsoft.SqlServer.TransactSql.ScriptDom from 180.59.2 to 180.78.1 Bumps SQLitePCLRaw.bundle_e_sqlite3 from 3.0.4 to 3.0.5 --- updated-dependencies: - dependency-name: Microsoft.SqlServer.TransactSql.ScriptDom dependency-version: 180.78.1 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: Microsoft.SqlServer.TransactSql.ScriptDom dependency-version: 180.78.1 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: Microsoft.SqlServer.TransactSql.ScriptDom dependency-version: 180.78.1 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: Microsoft.SqlServer.TransactSql.ScriptDom dependency-version: 180.78.1 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: SQLitePCLRaw.bundle_e_sqlite3 dependency-version: 3.0.5 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor ... Signed-off-by: dependabot[bot] <support@github.com>
…PlanShare/dev/patch-and-minor-dded2de96d deps: Bump Microsoft.SqlServer.TransactSql.ScriptDom and SQLitePCLRaw.bundle_e_sqlite3
…nViewer.App/dev/multi-daa5a8fc91 deps: Bump ModelContextProtocol and ModelContextProtocol.AspNetCore
Bumps Avalonia from 11.3.18 to 11.3.19 Bumps Avalonia.Desktop from 11.3.18 to 11.3.19 Bumps Avalonia.Fonts.Inter from 11.3.18 to 11.3.19 Bumps Avalonia.Themes.Fluent from 11.3.18 to 11.3.19 Bumps ModelContextProtocol from 2.0.0 to 2.1.0 Bumps ModelContextProtocol.AspNetCore from 2.0.0 to 2.1.0 --- updated-dependencies: - dependency-name: Avalonia dependency-version: 11.3.19 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Desktop dependency-version: 11.3.19 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Fonts.Inter dependency-version: 11.3.19 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Themes.Fluent dependency-version: 11.3.19 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: ModelContextProtocol dependency-version: 2.1.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: ModelContextProtocol.AspNetCore dependency-version: 2.1.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: Avalonia dependency-version: 11.3.19 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Desktop dependency-version: 11.3.19 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Fonts.Inter dependency-version: 11.3.19 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Themes.Fluent dependency-version: 11.3.19 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: ModelContextProtocol dependency-version: 2.1.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: ModelContextProtocol.AspNetCore dependency-version: 2.1.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor ... Signed-off-by: dependabot[bot] <support@github.com>
…nViewer.App/dev/patch-and-minor-7f3ba78d27 deps: Bump the patch-and-minor group with 6 updates
Interactive Microsoft Entra MFA could not work in Studio on Windows at all. Current Microsoft.Data.SqlClient (7.0.2 here) routes Authentication=ActiveDirectoryInteractive through the Windows WAM broker, and WAM requires the application to supply the HWND that will own its account picker. ServerConnection.BuildConnectionString set the authentication mode but nothing in the repo ever registered an auth provider, so instead of a prompt users got "0xwindow_handle_required / A window handle must be configured" and the connection failed. Not tenant-specific: broken for every Windows user on a current build. Studio 1.4.3 worked because it predates WAM being the default and fell back to a browser. Reported by joshdbe, who supplied the diagnostic that made it findable: 1.4.3 works, 1.19.1 does not. Same defect in PerformanceMonitor Lite (erikdarlingdata/PerformanceMonitor#2184); doing Studio first because that is the tool the reporter actually uses. EntraInteractiveAuth.Register takes a handle provider and installs an ActiveDirectoryAuthenticationProvider. Three deliberate choices: - Registered ONCE at app startup rather than per connection. SqlAuthenticationProvider installs against the authentication METHOD, not a connection, so one call covers every SqlConnection the process opens. Studio opens them from several unrelated places (connection dialog, query session control, schema service) and threading a handle through all of them is a change every future call site could forget. - WINDOWS ONLY. WAM does not exist on macOS or Linux, where interactive auth already works through the system browser and needs no handle, so registering a handle-supplying provider there would add a failure mode to the platforms Avalonia exists to serve. - The handle is resolved PER PROMPT, not captured at startup, and prefers the active window over the main window. A platform handle is not valid until its window is sourced, and a connection is usually triggered from the connection dialog - parenting the picker to the main window behind it would let the picker appear behind the dialog the user is looking at. The CLI gets a clear refusal instead of MSAL's error. planview accepts --auth entra and has no window to give, so it would have failed deep inside MSAL with a message that tells the operator nothing. It now says so up front and names the modes that work headless. Deliberately NOT silently substituting another auth mode: connecting as a different identity than the one asked for is worse than refusing. Whether the CLI should grow device-code flow is the open question on #425 rather than something to guess at here. Tests cover what is verifiable without a tenant: null handle provider rejected at wiring time rather than at prompt time, the non-Windows contract (Register declines, the provider is never consulted, and IsSupported stays TRUE so the CLI guard does not refuse entra on platforms where it works), and idempotent registration, since SetProvider is process-wide and a second call would otherwise silently replace the first once the SSMS extension wires in too. Whether the picker actually authenticates can only be established against a real Entra tenant on an Azure VM; the reporter has offered to test that and this is not claimed as verified until he does. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
PlanViewer.Core.Tests can wedge in a CoreCLR GC-suspension livelock on macOS ARM64
(.NET 10.0.8) and spin a full core until someone notices the heat. It happened
twice unprompted today: three hosts at ~108% CPU each, one of them wedged 22
minutes and still going when sampled. Root-caused from a sample: SuspendEE present
in all 2451 samples, the victim thread interrupted where CheckActivationSafePoint
can never succeed, and the mach-exception thread churning thread_get_state /
thread_set_state forever.
The load-bearing fact is that nothing inside the test host can stop it. xUnit
timeouts and in-test CancelAfter cannot fire because the execution engine itself is
suspended - that IS the livelock - and dotnet-stack and EventPipe hang for the same
reason. Only an out-of-process watchdog works, and both mechanisms here are
enforced by vstest.console, a separate process from the host it watches.
Two layers, deliberately:
- The three CI invocations (ci, nightly, release) now pass the blame-hang options,
which is what produces a sequence file naming the test that wedged.
- RunSettingsFilePath in the test csproj points at hang-watchdog.runsettings, so a
bare `dotnet test` with no flags gets a TestSessionTimeout backstop. This is the
layer that matters most: BOTH incidents were plain local runs, so a fix living
only in the workflow files would protect the machine that was never at risk and
leave the laptop unprotected.
15 minutes is far above any real runtime (individual tests are milliseconds, the
whole suite normally well under a minute), so it only fires on a genuine wedge.
Verified rather than assumed, because a silently-ignored runsettings would look
exactly like a working one: temporarily set TestSessionTimeout to 1ms and a bare
`dotnet test` reported "Aborting test run: test run timeout of 1 milliseconds
exceeded", then restored it. The blame collector confirms itself active on a normal
run ("All tests finished running, Sequence file will not be generated"). Zero test
hosts survived any run.
Two XML-comment errors on the way in, both caught by the toolchain rather than by
me: '--' is illegal inside an XML comment, and I had written the flag names and a
dash-separated aside into comments in both the csproj and the runsettings.
Evidence for a future dotnet/runtime report is preserved outside the session
scratchpad at ~/Documents/dotnet-hang-evidence (sample, gzip, and a README
explaining the signature); related to dotnet/runtime#66759.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Out-of-process watchdog for the macOS ARM64 test-host GC-suspension livelock. Verified by forcing a 1ms TestSessionTimeout abort on a flagless dotnet test.
…efusal Two findings from the PR review: - ActiveWindowHandle is called by MSAL from whatever thread SqlClient's token acquisition runs on, while desktop.Windows is a UI-thread-owned collection that can be mutated mid-enumeration by a dialog opening or closing. Marshal the lookup through Dispatcher.UIThread (safe: no connection path blocks the UI thread on auth - every open in the app is async), and degrade to IntPtr.Zero if the dispatcher can't deliver, which is MSAL's normal no-handle failure rather than a new one. - The CLI's --auth entra refusal had no test. Pin it with a real call through BuildServerConnection, made deterministic by an internal test-only reset of the process-wide one-way registration flag (the registration tests do run first in this process, so a skip-if- contaminated guard would have skipped on exactly the platform where the branch is reachable). The two test classes share a collection so the global state can't flip mid-test. Also drop using directives already provided as global usings in both projects, which the IDE flags on every touch of these files. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Give the WAM broker a parent window handle so Entra MFA works (#425)
System.Text.Json defaults MaxDepth to 64 and throws past it. An operator
tree nests once per operator and each level costs two JSON levels (the
object, then the Children array), so a plan roughly 30 operators deep
exhausts the default. The throw came out of an Avalonia click handler,
which Avalonia does not guard, so the process died with no dialog and
nothing the user could act on.
The reported stack named the cause precisely: the path was
$.Statements.OperatorTree.Children.Children...(30 deep)...NodeId - thirty
CONSECUTIVE Children. That distinction decides the fix, because the
exception message offers two causes ("either be due to a cycle or if the
object depth is larger than the maximum allowed depth of 64") and they
want opposite remedies. The serialized type is OperatorResult, which has
Children and no parent link, so it is a tree and depth is the right read.
Worth recording for whoever hits this next: the INTERNAL model it is
mapped from, PlanNode, DOES carry a Parent back-reference that the parser
populates (ShowPlanParser.RelOp.cs). Serializing that type directly would
be a genuine cycle needing [JsonIgnore], not a bigger number.
The ceiling is now one shared constant, because four places serialize this
object and none of them could tell when they got it wrong: both Robot
Advice entry points (QuerySessionControl and MainWindow build the payload
independently), the MCP tool options, and the CLI's two option sets. Only
the reported one crashes the process; the others returned an error where
the caller expected a plan.
1024 is about 500 nested operators against the ~30 that used to fail, and
it is deliberately paired with a caller-side catch at both UI sites: a
serialization limit should never be able to take the app down, so the
headroom is not the only thing standing between a deep plan and a crash.
Tested: 137/137 in PlanViewer.Core.Tests, including a reproduction that
fails on the old inline options and passes on the shared ones. App and CLI
build with zero warnings.
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bumps Avalonia from 11.3.19 to 11.3.20 Bumps Avalonia.Desktop from 11.3.19 to 11.3.20 Bumps Avalonia.Fonts.Inter from 11.3.19 to 11.3.20 Bumps Avalonia.Themes.Fluent from 11.3.19 to 11.3.20 Bumps Microsoft.AspNetCore.Components.WebAssembly from 10.0.10 to 10.0.11 Bumps Microsoft.AspNetCore.Components.WebAssembly.DevServer from 10.0.10 to 10.0.11 Bumps Microsoft.Data.Sqlite from 10.0.10 to 10.0.11 Bumps Microsoft.NET.Test.Sdk from 18.8.1 to 18.9.0 Bumps ModelContextProtocol from 2.1.0 to 2.2.0 Bumps ModelContextProtocol.AspNetCore from 2.1.0 to 2.2.0 Bumps System.CommandLine from 2.0.10 to 2.0.11 --- updated-dependencies: - dependency-name: Microsoft.Data.Sqlite dependency-version: 10.0.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia dependency-version: 11.3.20 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Desktop dependency-version: 11.3.20 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Fonts.Inter dependency-version: 11.3.20 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Themes.Fluent dependency-version: 11.3.20 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: ModelContextProtocol dependency-version: 2.2.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: ModelContextProtocol.AspNetCore dependency-version: 2.2.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: System.CommandLine dependency-version: 2.0.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Microsoft.AspNetCore.Components.WebAssembly dependency-version: 10.0.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Microsoft.AspNetCore.Components.WebAssembly.DevServer dependency-version: 10.0.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia dependency-version: 11.3.20 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Desktop dependency-version: 11.3.20 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Fonts.Inter dependency-version: 11.3.20 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Avalonia.Themes.Fluent dependency-version: 11.3.20 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor - dependency-name: Microsoft.NET.Test.Sdk dependency-version: 18.9.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: ModelContextProtocol dependency-version: 2.2.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: ModelContextProtocol.AspNetCore dependency-version: 2.2.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: patch-and-minor - dependency-name: System.CommandLine dependency-version: 2.0.11 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: patch-and-minor ... Signed-off-by: dependabot[bot] <support@github.com>
#437) Rule 12 flagged any predicate whose text contained CONVERT_IMPLICIT, and told the user it "prevents an index seek". Which side the conversion is on decides whether that is true, and the rule never looked. Data type precedence picks the side, and SQL Server converts the LOWER-precedence one. A numeric(18,0) column compared to an integer parameter converts the PARAMETER up: [db].[dbo].[t].[UNIV_SEZIONI]=CONVERT_IMPLICIT(numeric(18,0),[@0],0) The column is untouched, so it is still seekable, and SQL Server agrees: it emits no PlanAffectingConvert warning on that plan, which is why SSMS shows no warning icon while we showed one. The damaging shape is the mirror image, CONVERT_IMPLICIT(nvarchar(40),[db].[dbo].[t].[DisplayName],0)=[@d], where every row has to be converted before it can be compared. Reproduced rather than reasoned about, because the seekability claim is the whole question. Captured two actual plans of the reporter's repro from SQL Server 2025 under PARAMETERIZATION FORCED. Without an index on the search column it is a scan; ADD an index on that same column and SQL Server produces an Index Seek whose seek predicate is CONVERT_IMPLICIT(numeric(18,0),[@0],0) — it seeks straight through the conversion. That settles it: the conversion was never what prevented the seek. The scan fixture is committed as the test plan. So the check reads what is INSIDE the conversion rather than splitting on the comparison operator: the first argument is the target type and carries no brackets, so a bracket-qualified column reference in the remainder is the conversion input. Balanced-paren extraction is needed because the target type has parentheses of its own — numeric(18,0), varchar(50) — so the first ')' is not the end of the argument list. Second change, and it is load-bearing rather than tidying: the function-on-column check now walks every regex match instead of only the first. A parameter-side conversion now falls through to that check and is skipped there as benign, so with only the first match a conversion sitting to the LEFT of a genuine function-on-column would hide it — trading the fixed false positive for a new false negative. Verified by reverting just that hunk: Rule12d then fails with "Collection was empty". Behavior change, stated plainly: plans carrying a parameter-side conversion on a scan lose their "Non-SARGable Predicate" warning. The reporter's plan now reports Rule 11 instead — "scan with residual predicate, 50 of 50,000 rows survived filtering, check that you have appropriate indexes" — which is the true and actionable read of that plan. Nothing silently disappears. The existing corpus is untouched: regenerating WarningBaseline.txt across all 38 committed plans changes nothing except the two lines each for the two new fixtures. No committed plan was relying on the old verdict. Tested: 71/71 in the analyzer classes (PlanAnalyzerTests, WarningCharacterizationTests, PlanAnalysisPipelineTests, OperatorSelfTimeTests), plus 130 more across the other classes. Both new tests fail on the unfixed rule — Rule12c with "Collection was not empty", Rule12d with "Filter not matched". dotnet build is clean, 0 warnings. Not fixed here and not caused here: a full `dotnet test` does not finish on macOS ARM64 — it wedges inside McpPlanPathPolicyTests with every thread blocked. It is NOT the GC-suspension livelock from 5f84116 (no SuspendEE, 0% CPU, not spinning), --blame-hang-timeout does not fire on it, and it reproduces on unmodified dev. CI is ubuntu-latest, which is why it is invisible there. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
#430 was fixed by making the depth ceiling a shared constant and referencing it from every serializer that writes an AnalysisResult. That enumeration missed two: QueryStoreCommand builds its own JsonOptions and CompactJsonOptions, identical in shape to AnalyzeCommand's, and neither ever got MaxDepth. So `planview querystore` has kept failing on any plan deeper than roughly 30 operators for as long as `analyze` has been fine. It fails quietly, which is why nobody has reported it. The per-plan try/catch in the sweep loop turns the JsonException into one "ERROR: ..." row in summary.txt and moves to the next plan, so a Query Store sweep over a few hundred plans silently analyzes all but the deep ones. Query Store batch analysis is exactly where the deep plans are. The interesting part is WHY it was missed, because that decides the fix. A shared constant only helps the call sites that remember to reference it, and the options themselves were still duplicated per command — so editing AnalyzeCommand's two sets to add MaxDepth did nothing for the identical pair one file over, and nothing anywhere said they were supposed to match. The options now live on AnalysisJson next to the constant, as IndentedWithoutNulls and CompactWithoutNulls, and both commands point at them. There is no longer a copy to forget. Values are unchanged for analyze: same WriteIndented, same JsonIgnoreCondition.WhenWritingNull, same ceiling. Only querystore's behavior changes, and only from "fails on deep plans" to "does not". The test walks the options instead of trusting the call sites, since trusting them is what went wrong: every static JsonSerializerOptions on a *Command type in PlanViewer.Cli must carry the ceiling. Verified by reverting just the QueryStoreCommand hunk, at which point it fails naming both offenders — "QueryStoreCommand.JsonOptions (MaxDepth 0), QueryStoreCommand.CompactJsonOptions (MaxDepth 0)". A new command that rolls its own now fails here rather than in someone's Query Store sweep. Tested: 82 tests across AnalysisJsonDepthTests and the CLI/analysis classes, 0 failures. dotnet build clean, 0 warnings. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reporter could not tell our advice apart from the engine's, and there was no way to: both arrive as a PlanWarning carrying nothing but type, severity and message, and both render identically. 14 warning constructions in ShowPlanParser.Warnings.cs are lifted straight out of the plan's own <Warnings> element; 47 across PlanAnalyzer*.cs are our rules reading plan shape. Nothing in the model or the output said which was which. The distinction is not cosmetic. A warning SQL Server wrote into the plan is a record of what the engine did — it spilled, it converted, it had no statistics. One of our rules is an inference, and an inference can be wrong about a particular plan in a way the engine's own record cannot be. The same issue supplies the example: we told this reporter a conversion prevented an index seek on a plan SQL Server had raised no conversion warning about at all, and being able to see "that line is ours, this line is the engine's" is what would have let him weigh the two himself. PlanWarning.Source defaults to PerformanceStudio, and everything the parser produces is stamped SqlServer in ONE place — the single return of ParseWarningsFromElement, which every parser warning already funnels through. Stamping the 14 construction sites individually would have been the same bug as #430's missed QueryStoreCommand: a rule you have to remember at each site is a rule that eventually gets forgotten. A new engine warning added to that method is attributed correctly without anyone thinking about it. UX, since it was left to me: - Only the ENGINE's warnings are tagged, as " [SQL Server]", in both the GUI warnings panel and the CLI text output. Tagging both kinds would put a badge on every line and carry no information — our own advice is what a reader already expects from a plan analyzer, so the marked case should be the exception. - The tag sits next to the existing " [legacy]" tag and is built the same way, because that badge already established the idiom here. - JSON and MCP consumers get it as a "source" field rather than having to parse a tag out of a string. CLI output contract, flagged rather than slipped in: adding "source" to every warning changes the bytes of `analyze --compact`, so HistoricalCliContractTests.ExpectedCompactOutputSha256 is rolled. The change is additive — nothing removed or renamed, so a consumer reading fields by name is unaffected — but anything hashing or diffing whole output will see a difference. That constant exists to make this a decision instead of a discovery, so it is called out here rather than quietly updated. The characterization baseline is untouched: it digests type, severity and message, none of which changed. Tested: 65 across WarningSourceTests, HistoricalCliContractTests, WarningCharacterizationTests and PlanAnalyzerTests, 0 failures. The new tests pin both kinds on one plan that carries both, that no warning type is ever produced as both kinds across every committed plan, that only the engine's are tagged in text output, and that the JSON field is populated. dotnet build clean. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…PlanShare/dev/patch-and-minor-87032c3abd deps: Bump the patch-and-minor group with 11 updates
…443) * Stop corrupting memory resolving an open file handle on macOS (#441) fcntl(2) is variadic - int fcntl(int, int, ...) - and this called it through a plain DllImport with a fixed third parameter: [DllImport("libc", EntryPoint = "fcntl", SetLastError = true)] private static extern int Fcntl(int fileDescriptor, int command, byte[] buffer); On Apple arm64 the variadic ABI differs from AAPCS64: named arguments go in registers, variadic arguments go on the STACK. A fixed-signature P/Invoke puts the buffer in x2, and the callee never looks there. It does not fail, which is why nothing caught it. Reproduced standalone, outside the test suite, one open file: Arch : Arm64 FIXED rc=0 errno=0 len=0 -> '' rc=0 is fcntl reporting SUCCESS. F_GETPATH on success writes the path into the buffer it was handed; ours came back empty, so it wrote up to MAXPATHLEN bytes through whatever pointer happened to be in that stack slot. An arbitrary ~1KB write, on every call, into this process. That is the whole of #441. `dotnet test` has been unrunnable on macOS - it wedged, sometimes as a GC-suspension livelock spinning a core, sometimes as an all-threads-blocked deadlock, which is exactly what an arbitrary write into runtime memory looks like from the outside. Both shapes are gone. Diagnosed as a prediction rather than described afterwards: the three tests in McpPlanPathPolicyTests that pass are precisely the three that never reach GetFinalPath, and both that reach it hung, deterministically, 5/5 and 2/2. It is also why CI never saw it - Linux takes the /proc/self/fd branch and Windows takes GetFinalPathNameByHandle, so only macOS goes anywhere near fcntl, and only arm64 has the mismatch. ubuntu-latest structurally cannot reproduce this. The fix is libproc's proc_pidfdinfo, which answers the same question with a FIXED signature, so ordinary marshalling is correct. .NET has no varargs P/Invoke on this target at all - __arglist throws "Vararg calling convention not supported" - so repairing the fcntl declaration in place is not available. Worth recording, because I recommended the wrong fix on the issue first and it sounds right: comparing fstat(fd) against stat(path) by device+inode does NOT work here. stat() re-resolves the path, so if a symlink was swapped before the open it follows the swap too and the inodes match. It detects nothing. Resolving the descriptor back to its real path is the only thing that answers "where does this handle actually live", which is the question the TOCTOU check exists to ask. The struct offsets are derived in a comment from <sys/proc_info.h> rather than being magic numbers, and the returned size is checked against the expected 1200 instead of trusted - so a layout change in a future macOS fails loudly rather than quietly handing back the wrong bytes, which is the failure mode this file just came out of. Also note macOS answers with the CANONICAL path: /private/var/... where Path.GetTempPath() reports /var/... The roots are canonicalized through ResolveLinkTarget already, so containment still matches; the new test compares by identity rather than by string for the same reason. The old test asserted on the returned handle's Label and passed happily on Linux and Windows while this call had never once worked on Apple silicon. The new test asserts the thing that was actually broken - that the resolver returns the real path of the file that is genuinely open - on whatever platform it runs. Security note: GetFinalPath is the TOCTOU re-validation, confirming the path the kernel really opened is still inside the advertised roots. On macOS arm64 it has never performed that check. It failed CLOSED - the empty string reached Path.GetFullPath, which throws ArgumentException, which is not in OpenAsync's catch filter - so this was not an exploitable bypass, but the guarantee OpenAsync_ValidatesAndReturnsTheSameOpenedHandle claims to prove was not being provided, and the FileStream leaked on the way out. Tested: full `dotnet test` on macOS ARM64 now finishes in 16 seconds, 305 passed, 0 failed, 2 skipped, twice. It did not finish at all before. McpPlanPathPolicyTests went from 5/5 hangs to 3/3 passes. General rule this earns: no variadic libc function through a plain DllImport in this repo - fcntl, open, ioctl, the printf family. They look fine on x64 and silently corrupt memory on Apple silicon. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Write down the variadic P/Invoke rule that #441 earned The defect cost months of unrunnable tests on macOS and was invisible to CI, and nothing in the repo said not to do it. The specific trap is that it fails silently in both directions: fcntl reports success, and the corruption lands somewhere else entirely, so the crash never points at the call that caused it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…434) (#442) Dependabot #434 fails to build, and not because of anything it did wrong: Microsoft.Testing.Platform.MSBuild.targets(320,5): error : Testing with VSTest target is no longer supported by Microsoft.Testing.Platform on .NET 10 SDK and later. xunit.v3 4.0.0 drops VSTest on the .NET 10 SDK. There is no version of this bump that keeps the old runner, so it is migrate or stay on 3.2.2. #433 is the same family and passes only because bumping the VSTest adapter alone changes nothing. The opt-in is repo-level, not per-project, which the target names outright (_SupportsGlobalJsonTestRunner): global.json now selects the runner. Setting TestingPlatformDotnetTestSupport in the csproj alone does not do it, and I tried that first. xunit.runner.visualstudio is dropped because it IS the VSTest adapter and has no role here any more. That moots #433. The watchdog from 5f84116 needed porting, and this is the part worth reading. Both of its layers were VSTest-only: RunSettingsFilePath/TestSessionTimeout for bare local runs, and the blame-hang options in the three workflows. The local layer is now TestingPlatformCommandLineArguments carrying a 15m session timeout, same value and same no-flags-to-remember property; the workflows pass the hangdump options, which produce the same name-the-wedged-test artifact. I verified the local layer the way the original was verified, because a silently ignored property looks exactly like a working one: set it to 1ms temporarily and a bare `dotnet test` cancelled with 0 tests succeeded, then restored it. What the port does NOT recover, stated plainly because it would be easy to leave implied: neither MTP mechanism stops the GC-suspension livelock the original watchdog was written for. Measured against a genuinely wedged host on macOS ARM64, not assumed - it ran 2m55s at 107% CPU against a 90s hangdump timeout, and 8 minutes against a 60s session timeout, ignoring both. The reason is the same one 5f84116 gives for xUnit timeouts: the diagnostics the platform needs are served by the execution engine that is suspended. MTP does put the timeout in a separate controller process, which was the property that mattered, and it still cannot win. An ordinary hang, where the runtime is responsive, is killed fine. So this is not a regression against VSTest so much as both runners being equally powerless there, and I want to be careful not to claim more: I did not prove the OLD TestSessionTimeout could kill a livelocked host either. It was verified at 1ms on a healthy run, never against a real wedge. Filed separately. Tested: 279 passed, 0 failed, 2 skipped under the new runner (the whole suite except the Mcp/Repl classes, which wedge on macOS for reasons that predate this and are the subject of that separate issue). The CI flag form was run verbatim to confirm the options parse. dotnet build clean; the 9 warnings are the pre-existing MCP9005 obsolete-API uses in McpSmokeTests.cs. Two XML-comment '--' errors on the way in, exactly as 5f84116 warned. Noted in the comment so the next person does not rediscover it. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Minor rather than patch. 1.19.x would understate it: #439 adds a "source" field to every warning in the JSON and MCP output and a new badge in the app and CLI, and #437 changes what an existing analysis rule concludes about a plan. Both are things a consumer can notice, and one of them is output-shape. What ships: - #437 Rule 12 no longer calls a conversion non-SARGable when it converts the parameter rather than the column. Plans carrying a parameter-side conversion on a scan lose that warning and report the scan's residual predicate instead. Verified against all 38 committed plans: no other plan's verdict moves. - #439 SQL Server's own warnings are now told apart from ours, tagged [SQL Server] in the app and CLI and carried as "source" in JSON/MCP. Additive, but it changes the bytes of analyze --compact. - #431 Robot Advice no longer takes the app down on a deep plan. - #438 querystore gets the same depth ceiling analyze got; it had been failing quietly on deep plans, one ERROR row per plan. - #443 the macOS handle resolver no longer corrupts memory on Apple silicon. - #425 Entra MFA works again (WAM parent window handle). Not user-facing but worth knowing for anyone building from this tag: the suite runs on Microsoft.Testing.Platform now (#442), and `dotnet test` finishes on macOS for the first time (#443) - 307 tests, 305 passing, 13 seconds. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Release v1.20.0. Merging this fires
release.yml: signs the Windows binaries through SignPath and publishes the GitHub release.User-facing
numeric(18,0)column compared to an integer parameter converts the parameter and leaves the column seekable. Plans carrying a parameter-side conversion on a scan lose that warning and report the scan's residual predicate instead. Verified against all 38 committed plans — no other plan's verdict moves.[SQL Server]in the app and CLI, carried assourcein JSON and MCP output. Additive, but it changes the bytes ofanalyze --compact.querystoregets the same depth ceilinganalyzegot. It had been failing quietly on deep plans — oneERRORrow insummary.txtper plan — for as long asanalyzehad been fine.fcntlis variadic and was being called through a fixed-signatureDllImport, which on arm64 wrote up to 1KB through an arbitrary pointer on every call.Build and test
xunit.v34.0.0 dropping VSTest on the .NET 10 SDK.dotnet testfinishes on macOS for the first time: 307 tests, 13 seconds. It previously did not complete at all.Closes on release
#430, #435, #436.
Verification
Full suite run on all three platforms, not just CI's:
release.ymlruns on)The Windows run was deliberate:
ci.ymlandnightly.ymlare bothubuntu-latest, so #442's new Microsoft.Testing.Platform flags had never executed on Windows, and the release job would have been the first time. Proved with a throwaway push-triggered job running the exactrelease.ymlcommand before opening this.dotnet buildclean. The 9 warnings are the pre-existingMCP9005obsolete-API uses inMcpSmokeTests.cs.🤖 Generated with Claude Code