Skip to content

Match strong-name identity when resolving PSES dependencies#2303

Open
andyleejordan wants to merge 1 commit into
mainfrom
andyleejordan/alc-isolation-investigation
Open

Match strong-name identity when resolving PSES dependencies#2303
andyleejordan wants to merge 1 commit into
mainfrom
andyleejordan/alc-isolation-investigation

Conversation

@andyleejordan

@andyleejordan andyleejordan commented Jun 12, 2026

Copy link
Copy Markdown
Member

Summary

A focused trial of tightening how PsesLoadContext decides whether a candidate assembly can satisfy a dependency request. Patrick and I have suspected for a while that our version-only matching is inadequate; this proposes we trial requiring the full strong-name identity to match.

Problem

On .NET Core, PsesLoadContext.IsSatisfyingAssembly gates whether a DLL in $PSHOME or our bundled Common directory can satisfy a dependency request, using only:

  • the simple name (case-insensitive), and
  • candidate.Version >= required.Version.

That ignores the rest of the assembly identity. A same-named assembly with a different public key token (i.e. a genuinely different assembly) was treated as a drop-in replacement, and the mismatch only surfaced later as a FileLoadException/TypeLoadException at bind time instead of being declined up front.

Change

Also require:

  • Public key token — if the reference is strong-named, the candidate's token must match exactly; a non-strong-named reference imposes no token requirement.
  • Culture — must match, so we never substitute a satellite resource assembly for the neutral one (or vice versa).

The pure comparison moved into an internal static overload taking two AssemblyNames so it can be unit-tested without DLLs on disk.

Why this is safe to trial

  • The check can only return false in more cases, and only for assemblies that could not have satisfied the reference anyway.
  • On a mismatch we decline to short-circuit and fall through to the default load context's own (laxer) resolution — equivalent to returning "not mine".
  • Measured against a current build, no presently-bundled dependency changes resolution under the new rules (zero token mismatches across Common vs $PSHOME), so today this is purely added protection.

Tests

PsesLoadContextTests (net8.0 / CoreCLR only, since the Hosting assembly is .NET Core only) covers exact match, newer/older version, name case-insensitivity, differing public key token, strong-named-vs-unsigned, no-required-token, and culture match/mismatch. All 10 pass; net462 still compiles (reference and tests are guarded).

Context / scope

Part of the broader ALC isolation investigation behind the recurring "Assembly with same name is already loaded" / "Could not load file or assembly" reports. This is the resolver-correctness piece only — it does not by itself address the feature-side eager-loading (completion / Get-Help importing user modules) or the Windows PowerShell "no ALC at all" class of issues.

Open questions

  • Whether to also tighten the Version >= rule (e.g. require major-version compatibility) — deliberately left as-is here.
  • Whether the trial framing is enough to merge as-is, or if we'd rather gate it.

🤖 Drafted by Copilot (Claude Opus 4.8) for @andyleejordan to review and edit before merging.

@andyleejordan
Match strong-name identity when resolving PSES dependencies
0c0e9da 
`PsesLoadContext.IsSatisfyingAssembly` decided whether a candidate DLL in
`$PSHOME` or our bundled `Common` directory could satisfy a dependency
request using only the simple name and `Version >=`. That ignores the rest
of the assembly identity, so a same-named assembly with a different public
key token (i.e. a genuinely different assembly) was treated as a drop-in
replacement. When the runtime then bound against it, the mismatch surfaced
later as a `FileLoadException`/`TypeLoadException` rather than being declined
up front. Patrick and I had suspected for a while that the version-only
matching was inadequate, so this is a focused trial of tightening it.

We now also require the public key token and culture to match:

- If the requested reference is strong-named, the candidate's public key
  token must match exactly; a non-strong-named reference imposes no token
  requirement.
- The culture must match, so we never substitute a satellite resource
  assembly for the neutral one (or vice versa).

The check can only return `false` in more cases than before, and only for
assemblies that could not have satisfied the reference anyway. On a token
mismatch we now decline to short-circuit and fall through to the default
load context's own (laxer) resolution instead of forcing a copy that fails
at load. Measured against a current build, no presently-bundled dependency
changes resolution under the new rules, so this is purely added protection.

I pulled the pure comparison into an `internal` overload taking two
`AssemblyName`s and added `PsesLoadContextTests` covering the version, name,
public key token, and culture cases. The Hosting assembly (and thus
`PsesLoadContext`) is .NET Core only, so the project reference and tests are
guarded to `net8.0`/`CoreCLR`.

Drafted by Copilot (Claude Opus 4.8).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@andyleejordan andyleejordan marked this pull request as ready for review June 14, 2026 02:27
Copilot AI review requested due to automatic review settings June 14, 2026 02:27
Copilot AI reviewed Jun 14, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR tightens dependency resolution in the PsesLoadContext AssemblyLoadContext to avoid incorrectly treating same-named but differently strong-named assemblies as valid substitutes, preventing late runtime bind failures and improving resolver correctness.

Changes:

  • Refines PsesLoadContext.IsSatisfyingAssembly to require matching public key token (when the reference is strong-named) and matching culture, in addition to name + candidate.Version >= required.Version.
  • Extracts the identity comparison into an internal static overload that compares two AssemblyName instances (enabling direct unit testing).
  • Adds net8.0-only unit tests and wiring (project reference + InternalsVisibleTo) to validate strong-name and culture matching behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/PowerShellEditorServices.Hosting/Internal/PsesLoadContext.cs Tightens assembly “satisfies” logic to include public key token and culture matching, and factors it into a testable overload.
src/PowerShellEditorServices.Hosting/PowerShellEditorServices.Hosting.csproj Exposes hosting internals to the test assembly via InternalsVisibleTo for unit testing.
test/PowerShellEditorServices.Test/PowerShellEditorServices.Test.csproj Adds a net8.0-only project reference to the Hosting project so tests can target PsesLoadContext.
test/PowerShellEditorServices.Test/Session/PsesLoadContextTests.cs Introduces unit tests covering version/name matching, token matching rules, and culture matching.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@JustinGrote

JustinGrote commented Jun 15, 2026

Copy link
Copy Markdown
Collaborator

Would this have any issues with different PowerShell versions providing different base assemblies with different strongnames/publickey than what we are trying to match? That'd be my only concern.

@andyleejordan

andyleejordan commented Jun 15, 2026

Copy link
Copy Markdown
Member Author

@JustinGrote that's a solid question worth testing...

@andyleejordan

andyleejordan commented Jun 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

@JustinGrote Good news: tested and it holds up. Short answer — no, this doesn't cause issues across PowerShell versions, because a newer PowerShell changes the assembly version but never the strong-name (public key token). The matcher accepts newer versions (>=) and the token stays identical, so there are no false rejections.

How I tested

Built this branch, linked it into a sibling vscode-powershell, and ran the extension's full E2E suite (which starts a real PSES session) against each PowerShell, pointing the session at each binary via powershell.powerShellAdditionalExePaths + powerShellDefaultVersion. The 7.x binaries were downloaded from GitHub releases and extracted (not installed). Also did diagnostic-level direct launches to capture which ALC each assembly lands in.

PowerShell Runtime Result
7.4.16 .NET 8 E2E 112/112, PSES connected
7.5.7 .NET 9 E2E 112/112, PSES connected
7.6.2 .NET 10 E2E 112/112, PSES connected
5.1 .NET Framework loads cleanly (not affected — see below)

Plus the new PsesLoadContextTests pass 10/10.

The proof for your exact concern

Here's the same base assembly under .NET 8 vs .NET 10 — version bumps 8.0.0.0 → 10.0.0.0, public key token unchanged:

# 7.4.16 (.NET 8) — StartEditorServices log
Loaded into load context "Default" ...: System.Net.Http, Version=8.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a

# 7.6.2 (.NET 10) — StartEditorServices log
Loaded into load context "Default" ...: System.Net.Http, Version=10.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a
Loaded into load context "Default" ...: System.Text.Json, Version=10.0.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51

The well-known tokens (b03f5f7f11d50a3a, cc7b13ffcd2ddd51, 31bf3856ad364e35, adb9793829ddae60) are stable across .NET 8/9/10. PSES + its bundled deps go into the isolated context, $PSHOME/shared-framework assemblies resolve from Default — exactly as intended, with zero FileLoad/TypeLoad errors:

# 7.6.2 (.NET 10)
Loaded into load context "PsesLoadContext" ...: Microsoft.PowerShell.EditorServices, Version=4.6.0.0, ..., PublicKeyToken=null
Loaded into load context "PsesLoadContext" ...: Microsoft.Extensions.Logging.Abstractions, Version=10.0.0.0, ..., PublicKeyToken=adb9793829ddae60
PSES Startup Completed. Starting Language Server.

And the E2E run asserting the actual executable/version that answered, per version:

[crossver] PSES connected: version=7.4.16 arch=X64 exePath=...\pwsh\7.4.16\pwsh.exe   →  112 passing
[crossver] PSES connected: version=7.5.7  arch=X64 exePath=...\pwsh\7.5.7\pwsh.exe    →  112 passing
[crossver] PSES connected: version=7.6.2  arch=X64 exePath=C:\Program Files\PowerShell\7\pwsh.exe → 112 passing

Why the public-key-token check is still the right call

The token comparison only rejects a same-named but genuinely different assembly (different publisher key) — and in that case falling through to PSES's own bundled copy is the correct, safer behavior (that's the bug this PR fixes). It never rejects a legitimate, forward-compatible $PSHOME assembly, since those keep the same token.

Note on 5.1

PsesLoadContext is #if CoreCLR-only, so Windows PowerShell 5.1 isn't touched by this change at all — it still uses the net462 AssemblyResolve path. Confirmed with a direct launch: {"status":"started", ... "powerShellVersion":"5.1.26100.8655"}, deps resolved cleanly. (Side note: the headless vscode-test harness can't spin up a Windows PowerShell terminal session — an environment artifact unrelated to this change; pwsh 7.x works fine in the same harness.)

Drafted by Copilot, reviewed by @andyleejordan.

@andyleejordan

andyleejordan commented Jun 16, 2026

Copy link
Copy Markdown
Member Author

@JustinGrote @SeeminglyScience I think this is worth landing into a preview and hope it alleviates some of the assembly conflicts users run into.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@JustinGrote JustinGrote Awaiting requested review from JustinGrote

@SeeminglyScience SeeminglyScience Awaiting requested review from SeeminglyScience

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

Area-Engine Area-Startup Issue-Bug A bug to squash.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@andyleejordan @JustinGrote