fix: resolve SqlClient runtime asset selection (#50)

[potatoqualitee]

No description provided.

[claude]

Code Review — fix: resolve SqlClient runtime asset selection (#50)

Overview

This PR fixes a subtle but impactful bug: build.ps1 was copying the Unix SqlClient runtime asset over core/lib/Microsoft.Data.SqlClient.dll (the root/reference assembly), meaning Windows users were loading the wrong DLL variant at runtime. The fix removes that erroneous Copy-Item and replaces the naive single-path load with explicit runtime-RID-aware selection.

The approach is correct and the changes are well-scoped. A few things worth discussing below.

---

What's Working Well

• Root cause fix is clean: Removing the Copy-Item in build.ps1 that overwrote the root SqlClient is the right call. The hash-comparison validation added below it is a smart guard against this regressing silently.
• Get-DbatoolsSqlClientPath explicitly picks runtimes/win/lib/net8.0/ or runtimes/unix/lib/net8.0/ on Core before falling back to the root assembly — this is the correct NuGet runtime resolution order.
• GetArchitectureRid() and GetPlatformRid() are cleanly implemented with C# 7.3-compatible switch statements (no switch expressions). Good.
• Build validation steps in both CI workflows are a solid addition — failing fast if the wrong DLL is loaded beats debugging a runtime type-load failure.

---

Issues and Suggestions

1. build.yml verification step hardcodes Windows paths (no $IsWindows guard)

In build.yml, the verification step unconditionally checks for the Windows path:

$expected = "runtimes\win\lib\net8.0\Microsoft.Data.SqlClient.dll"

The build-release.yml version correctly wraps this in if ($IsWindows) { ... } else { ... }. If build.yml ever runs on a Linux runner (or the matrix is expanded), this step will always throw. Suggest applying the same conditional from build-release.yml.

2. GetManagedAssemblyPaths tries the root assembly first

paths.Add(Path.Combine(_libPath, fileName));          // root — tried first
paths.Add(Path.Combine(_libPath, "runtimes", platformRid, "lib", "net8.0", fileName));

For Microsoft.Data.SqlClient, the root core/lib/Microsoft.Data.SqlClient.dll is a reference assembly (or the wrong-platform build). If the OnResolving handler is ever triggered for SqlClient itself (e.g., a reload attempt), it would load the root over the runtime-specific variant. This won't happen in normal operation since Get-DbatoolsSqlClientPath loads SqlClient explicitly before the resolver matters — but it's a latent risk for other assemblies that ship both a root and a runtime-specific DLL. Consider placing platform-specific paths before the root path.

3. Redundant SetEnvironmentVariable call in Add-DbatoolsNativeSearchPath

$env:PATH = $nativePath + $pathSeparator + $env:PATH
[System.Environment]::SetEnvironmentVariable("PATH", $env:PATH, "Process")

In PowerShell, assigning $env:PATH = ... already calls the Win32 SetEnvironmentVariable under the hood — the explicit .NET call is a no-op here. Not harmful, but adds confusion about why it's needed. The first line is sufficient; the second can be removed.

4. netstandard1.6 fallback in GetManagedAssemblyPaths — probably dead code

paths.Add(Path.Combine(_libPath, "runtimes", platformRid, "lib", "netstandard1.6", fileName));

No package in the dependency tree (certainly not SqlClient 6.x) ships a netstandard1.6 runtime asset. This path will never resolve anything. Harmless, but worth removing to keep the search list minimal and readable.

5. GetPlatformRid() returns "unix" for macOS

This is intentional for SqlClient (which uses win/unix RIDs), but the OnResolving handler is general-purpose. Any package that ships osx-specific runtime assets wouldn't be found through platformRid. This is probably acceptable given the project's target audience (Windows + Linux CI), but worth a comment explaining that "unix" is SqlClient-specific and not a general RID.

6. Silent catch in OnResolvingUnmanagedDll swallows native load errors

catch
{
    // Failed to load, try the next candidate
}

NativeLibrary.Load can throw DllNotFoundException for missing dependencies (not just missing files), which the File.Exists check above won't catch. Swallowing all exceptions here means a DLL that exists but fails to load (wrong architecture, missing CRT dep) will silently fall through. At minimum, consider logging to Trace or the existing verbose mechanism before continuing.

---

Minor Nits

• The [CmdletBinding()] attribute on Get-DbatoolsSqlClientPath and Add-DbatoolsNativeSearchPath is unused — neither function writes verbose/warning output or uses pipeline input. Not wrong, but adds noise.
• End-of-file newlines were added to build.ps1 and dbatools.library.psm1 — good cleanup.

---

Summary

The core fix is correct and the validation additions are valuable. The main actionable items are:

1. Add $IsWindows guard to the build.yml verification step (mirrors build-release.yml)
2. Consider reordering GetManagedAssemblyPaths to prefer runtime-specific paths before the root assembly
3. Remove the redundant SetEnvironmentVariable call in Add-DbatoolsNativeSearchPath

Items 4–6 are lower priority. Overall this is a solid, well-understood fix and the build/CI additions reduce the chance of regression.

[claude]

Code Review — fix: resolve SqlClient runtime asset selection (#50)

Note: A prior automated review comment on this PR contains several inaccurate findings (fabricated code issues that don't exist in the actual diff). This review supersedes it with accurate observations.

---

Overview

The PR fixes a genuine bug: build.ps1 was copying the Unix SqlClient runtime asset over core/lib/Microsoft.Data.SqlClient.dll, causing Windows users to load the wrong DLL variant at runtime. The fix is:

1. Remove the erroneous Copy-Item in build.ps1
2. Add explicit runtime-RID-aware assembly selection via Get-DbatoolsSqlClientPath
3. Expand CoreRedirector to handle both managed (via OnResolving) and native (via OnResolvingUnmanagedDll) assembly resolution
4. Add a hash-comparison guard in build.ps1 to catch this regression
5. Add CI verification steps to both workflow files

---

What's Correct

• GetManagedAssemblyPaths order: Platform-specific paths (runtimes/win/ or runtimes/unix/) are tried before the root assembly. This is the correct NuGet runtime resolution order.
• Both CI workflow files (build.yml and build-release.yml) have the same correct if ($IsWindows) conditional for expected path selection.
• Replace('/', '\') normalization before the -like comparison in CI handles cross-platform path separators correctly — on Linux the Replace converts all / to \, making the comparison consistent.
• GetArchitectureRid/GetPlatformRid: Clean C# 7.3-compatible switch statements.
• Add-DbatoolsNativeSearchPath safely guards against double-adds to $env:PATH and against missing native directories.

---

Issues Worth Addressing

1. _libPath contains doubled backslashes on Windows (pre-existing, but newly relevant)

$dir = [System.IO.Path]::Combine($script:libraryroot, "lib")
$dir = ("$dir" + [System.IO.Path]::DirectorySeparatorChar).Replace('\', '\\')

The .Replace('\', '\\') was originally needed when $dir was embedded directly into the C# here-string as a string literal (where \\ represents a single \). Now that $dir is passed as a runtime parameter to Register(string libPath), the doubling is no longer correct: _libPath receives "C:\\path\\lib\\" with actual doubled backslashes.

The old code used direct concatenation (_libPath + name + ".dll") which happened to work with the trailing separator. The new Path.Combine(_libPath, ...) still produces valid-ish paths on Windows because NTFS normalizes redundant separators, but this is fragile and will confuse anyone debugging path issues.

Suggested fix (can be a follow-up PR):

$dir = [System.IO.Path]::Combine($script:libraryroot, "lib") + [System.IO.Path]::DirectorySeparatorChar

2. GetPlatformRid() and GetArchitectureRid() re-evaluated on every OnResolving call

These static methods re-inspect RuntimeInformation on every invocation. Since OS and architecture are constants at runtime, caching them as static readonly fields would be cleaner:

private static readonly string _platformRid = ComputePlatformRid();
private static readonly string _architectureRid = ComputeArchitectureRid();

Low practical impact since OnResolving only fires for assemblies that fail default resolution, but worth noting for hygiene.

3. OnResolvingUnmanagedDll intercepts all native loads in the AppDomain

The handler fires for every [DllImport] / NativeLibrary.Load call from any assembly in the process. It will probe lib/runtimes/<rid>/native/<name> for the library name. This is intentional for SNI resolution, but since it runs globally, a name collision between a dbatools-bundled native DLL and one another module expects from the system could cause the wrong binary to be loaded.

The current directory structure makes this low-risk in practice, but a short comment above the handler explaining the intentional scope (module-owned native assets only) would help future maintainers reason about this safely.

---

Minor Observations

• The GetNativeLibraryNames extension guards (EndsWith(".dll"), ".so", ".dylib") correctly prevent double-extension. The "lib" + name + ".so" variant covers Linux convention. No issues.
• Trace.TraceWarning for load failures is the right choice here — WriteVerbose can't be called from C# code running outside a PS runspace. Diagnostics will be visible to anyone who registers a TraceListener, which is the appropriate mechanism.
• The hash comparison in build.ps1 is an excellent guard. Well done.

---

Verdict

The root cause fix is correct and well-targeted. Issues #1 (doubled backslashes) is a pre-existing concern that the PR's new Path.Combine usage makes worth cleaning up. Issues #2 and #3 are minor. The CI verification additions are a solid investment. Overall this is a good PR.

[claude]

Code Review — fix: resolve SqlClient runtime asset selection (#50)

Note: A previous automated review on this PR contained several inaccurate findings (incorrect path-ordering claim, a non-existent SetEnvironmentVariable call, a non-existent netstandard1.6 path, and a wrong claim about build.yml missing an $IsWindows guard). This review is based on reading the actual diff.

---

Overview

This PR fixes a real bug: build.ps1 was calling Copy-Item to copy runtimes/unix/lib/net8.0/Microsoft.Data.SqlClient.dll over core/lib/Microsoft.Data.SqlClient.dll, meaning Windows hosts loaded the wrong platform DLL. The fix removes that copy and adds explicit runtime-RID-aware selection throughout the module loading path.

---

What's Working Well

• Root cause fix is clean: Removing the Copy-Item in build.ps1 directly eliminates the regression. No workaround, no band-aid.
• Get-DbatoolsSqlClientPath correctly selects runtimes/win/lib/net8.0/ or runtimes/unix/lib/net8.0/ before falling back to the root DLL — this matches NuGet's runtime resolution order.
• GetManagedAssemblyPaths tries platform-specific paths first, then falls back to the root (paths.Add(_libPath + fileName) is last). Ordering is correct.
• Add-DbatoolsNativeSearchPath is idempotent — it checks if the path is already present before prepending to $env:PATH.
• All C# code uses switch statements (not switch expressions), correctly respecting LangVersion 7.3.
• Hash-based validation in build.ps1 is a smart regression guard for the specific failure mode being fixed.
• OnResolvingUnmanagedDll safely returns IntPtr.Zero for any DLL not found under _libPath, so it won't interfere with system library resolution.
• Both workflow files correctly use if ($IsWindows) { ... } else { ... } in the verification step — they are symmetric.

---

Issues

1. Duplicated CI verification step (maintenance risk)

The 35-line verification block is copy-pasted verbatim into both build.yml and build-release.yml. If the expected path or SNI check logic ever changes, both files must be updated in sync.

Suggestion: Extract to a composite action at .github/actions/verify-sqlclient/action.yml:

# .github/actions/verify-sqlclient/action.yml
name: Verify SqlClient runtime asset
runs:
  using: composite
  steps:
    - shell: pwsh
      run: |
        # ... the shared block ...

Then both workflows call uses: ./.github/actions/verify-sqlclient. Not a blocker for this fix, but worth a follow-up.

2. net8.0 hardcoded in GetManagedAssemblyPaths

paths.Add(Path.Combine(_libPath, "runtimes", _platformRid, "lib", "net8.0", fileName));

Low risk today given the Microsoft.PowerShell.SDK 7.4.x ceiling in CLAUDE.md, but when that ceiling lifts this path breaks silently (the file won't exist, so it falls through to the root). A computed TFM or even a comment cross-referencing the SDK version constraint would help future maintainers notice the dependency.

3. OnResolvingUnmanagedDll fires on every native DLL load in the process

The handler is process-global and doesn't filter by requesting assembly. Every native DLL loaded by any code in the process (system DLLs, SqlClient's own interop, etc.) triggers GetNativeLibraryNames and up to 3 File.Exists probes into _libPath. The probes are fast and most OS-level DLL loads are infrequent, so this is unlikely to matter in practice — but it's worth noting if latency anomalies surface.

The existing comment acknowledges the global scope, which is good. No action required.

4. Get-DbatoolsSqlClientPath and GetManagedAssemblyPaths duplicate the same RID logic

Both independently encode the win/unix platform choice. The explicit PowerShell pre-load via Get-DbatoolsSqlClientPath is intentional (it runs before the resolver is registered), but if SqlClient's asset layout changes — e.g., a linux-specific managed DLL — both code paths would need updating independently.

A short comment in Get-DbatoolsSqlClientPath noting it must stay in sync with CoreRedirector.GetManagedAssemblyPaths would reduce the chance of a future one-sided update.

---

Minor Nits

• The new $criticalFiles entries in build.ps1 use \ path separators consistently with the surrounding entries. No issue.
• The build.ps1 hash check only validates the Unix→root overwrite scenario (not Windows→root). This is precisely targeted at the regression being fixed. Fine as-is.
• Removing the missing newline at end of both files is a clean housekeeping fix.

---

Summary

The core fix is correct and the approach is sound. The duplicated CI step is the only issue worth addressing before merge — everything else is low-priority or informational. The C# additions are idiomatic C# 7.3, the resolver ordering is correct, and the build validation gives good confidence this regression won't silently reoccur.

[claude]

Code Review — fix: resolve SqlClient runtime asset selection (#50)

Note: Two prior bot review comments exist on this PR with some inaccurate observations (e.g. references to SetEnvironmentVariable and netstandard1.6 that don't appear in this diff). This review is based directly on the actual diff.

---

Overview

This PR fixes a real bug: build.ps1 was calling Copy-Item to copy the Unix SqlClient runtime asset over core/lib/Microsoft.Data.SqlClient.dll (the root reference assembly), causing Windows users to load the wrong DLL variant. The fix:

1. Removes the erroneous Copy-Item in build.ps1
2. Adds platform-aware GetManagedAssemblyPaths + OnResolvingUnmanagedDll to CoreRedirector
3. Adds Get-DbatoolsSqlClientPath / Add-DbatoolsNativeSearchPath PowerShell helpers
4. Guards the regression with a SHA256 hash check in build.ps1 and a CI verification composite action

The approach is correct and the C# additions are fully C# 7.3-compatible (switch statements, no switch expressions, no nullable refs).

---

What's Working Well

• Root cause fix is clean: the Copy-Item removal and the hash-comparison regression guard are targeted and effective.
• GetManagedAssemblyPaths ordering is correct: platform-specific (win/unix) → architecture-specific (win-x64, etc.) → root. This matches NuGet's own RID resolution order.
• $dir cleanup: The old .Replace('\', '\\') was incorrectly escaping backslashes for C# string embedding — but $dir was always passed as a method argument, not embedded in the C# here-string. Removing it is a correctness fix.
• verify-sqlclient composite action: correctly uses $IsWindows for both build.yml and build-release.yml. Both workflows use the same composite action, so the $IsWindows guard applies uniformly — no issue here.
• C# 7.3 compliance: switch with fall-through in ComputeArchitectureRid, no switch expressions, no nullable references. ✓

---

Issues

1. Get-DbatoolsSqlClientPath and GetManagedAssemblyPaths duplicate the same RID logic

Both independently encode the win/unix platform selection for SqlClient. Get-DbatoolsSqlClientPath runs before the resolver is registered (to pre-load SqlClient), so the duplication is intentional — but if SqlClient's asset layout changes (e.g., osx-specific managed DLL), both paths would need updating independently. A comment cross-referencing them would reduce the chance of a future one-sided update.

2. OnResolvingUnmanagedDll is process-global

The handler fires for every native DLL loaded by any code in the process — system DLLs, SqlClient interop, etc. — triggering File.Exists probes into _libPath. The existing comment in the code acknowledges the global scope, which is good. The probes are cheap and infrequent, so this is acceptable as-is. Just confirming the acknowledgment is sufficient.

3. Swallowed exceptions in OnResolvingUnmanagedDll can hide real failures

catch (Exception ex)
{
    Trace.TraceWarning("Failed to load native library from '{0}': {1}", nativePath, ex.Message);
}

NativeLibrary.Load can throw DllNotFoundException for a file that exists but has missing dependencies (e.g., a CRT mismatch). File.Exists won't catch this — the code correctly logs to Trace, but since Trace output is not visible in normal PowerShell use, a subtle native load failure would be silent to end users. Low risk given the controlled DLL set, but worth noting.

---

Minor Nits

• [CmdletBinding()] on Get-DbatoolsSqlClientPath and Add-DbatoolsNativeSearchPath is unused — neither function writes verbose/warning/debug output or uses pipeline input. Not wrong, just noise.
• The build.ps1 hash check validates only the unix→root overwrite scenario (which is precisely the regression being fixed). Fine as targeted.
• Trailing-newline fixes on build.ps1 and dbatools.library.psm1 are clean housekeeping.

---

Summary

The core fix is correct, the approach is sound, and the regression guard is a meaningful safety net. The GetManagedAssemblyPaths ordering and the $dir cleanup are both genuine improvements over the previous code. The only actionable suggestion is a cross-reference comment between Get-DbatoolsSqlClientPath and GetManagedAssemblyPaths to help future maintainers keep them in sync. Everything else is low-priority or informational. Approachable to merge.