skyline: Port the native waters_connect/UNIFI readers to IdentityModel 7

[chambm]

Why

Updating Shared/Lib/IdentityModel.dll to 7.0.0.0 broke the native build. IdentityModel 7 removed the TokenClient constructor overloads and the AuthenticationStyle enum, and only the managed callers had been ported — UnifiData.cpp and WatersConnectData.ipp still built their token request the 3.9 way, so pwiz_vendor_api_unifi failed to compile. That cascades: pwiz_data_cli.dll, msconvert.exe and BlibBuild.exe all link that library, so none of them built either.

Sharing the request instead of restating it

OAuthPasswordGrantClient moves from CommonMsData down into CommonUtil, and both native readers now call it. This keeps the POST, the Basic authorization header and the protocol-error-vs-HTTP-error classification in exactly one place rather than growing a second C++/CLI copy.

CommonUtil is the right home because it has no ProjectReference of its own. Referencing CommonMsData from a native target would have closed a cycle back through ProteowizardWrapper → pwiz_data_cli → this very library.

UnifiAccount/UnifiSession also move onto HttpClientWithProgress, matching the shape #4613 established for waters_connect, and pick up using disposal along the way.

Build plumbing

pwiz.CommonUtil.dll is declared as a Jamfile target and referenced by target ID, not by path — <assembly> is a dependency feature whose values are virtual targets, so bjam builds it on demand.

It builds with MSBuild, not dotnet build: the SDK's MSBuild resolves cultures through ICU, which doesn't recognize the legacy zh-CHS of CommonUtil's satellite .resx files and so gives them the same manifest name as the neutral resources (MSB3577). Framework/VS MSBuild still uses NLS.

System.Text.Json.dll joins the vendored assemblies in Shared/Lib. IdentityModel 7 dropped Newtonsoft and parses the token response with it, so it's a plain runtime dependency for every consumer of the UNIFI reader — it sits alongside the other runtime-only entries already in .shared-assemblies (System.Buffers, Microsoft.Bcl.AsyncInterfaces, and so on).

msvc.jam

<assembly> was declared a dependency feature but never behaved like one. Every value had always been a prebuilt file, so nothing noticed — until one was generated by the build: set-assemblies put the paths into /FU without declaring an edge, and the compile raced ahead of the DLL (fatal error C1192: #using failed). copy-assemblies in the same file already declares that edge for the copy it makes; this adds the matching one for the compile.

Test fix

Skyline opens one GraphChromatogram per replicate. FindOpenForm deliberately asserts the form is unique, so the existing code only ever worked for a single-file import and failed TestWatersConnect with Multiple GraphChromatogram forms open simultaneously as soon as a second file was imported.

Each replicate's graph is now looked up by name, and the graphs are tiled first — a chromatogram graph that isn't showing draws no curves, so without tiling the second replicate reports CurveCount 0. Replicate names come from the document rather than _filenames, because ImportResultsNameDlg strips the common prefix and suffix (ID33140_03a_…/ID33141_03a_… become replicates named 0 and 1).

The curve count is now an explicit per-dataset expectation. The old CurveCount == _filenames.Length compared two unrelated quantities that happened to coincide.

Testing

Full native build green, from this branch's own source rather than borrowed binaries: pwiz_data_cli.dll, msconvert.exe, BlibBuild.exe, Skyline-daily.exe.

TestWatersConnect passes, verified over repeated runs against the live server. Authentication was confirmed end to end — no auth errors, tokens issued by the real identity server.

TestUnifi still fails, both before and after this change: it reaches the curve check while the live import is still running (AllChromatogramsGraph at 10%, No final cache), because WaitForDocumentLoaded() isn't covering the remote import. TestWatersConnect survives the same race only because its import finishes inside the 5s wait. Left alone here as a pre-existing, separate issue.

Also includes the stale RemoteApi path fix for the VCS trigger config (cherry-picked from #4632's 48dff2020d), without which changes to these very files don't trigger the TestConnected/Container CI.

🤖 Generated with Claude Code

https://claude.ai/code/session_0182oRXbeKQGEpF1kTkdQAsr

[Copilot]

🟡 Changes recommended

The updated UNIFI HTTP call sites should dispose HttpResponseMessage instances returned by SendRequest(), and there’s also a concrete XML documentation mismatch in the newly added OAuth helper.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR restores the native UNIFI / waters_connect reader build after upgrading the vendored IdentityModel assembly to v7 by centralizing the OAuth password-grant token request logic in a shared managed helper that both managed and C++/CLI callers can use. It also updates related UNIFI managed code to use HttpClientWithProgress consistently, fixes a functional test that assumed only one chromatogram graph form, and adjusts build/CI plumbing to correctly build and stage the new managed dependency.

Changes:

• Introduces OAuthPasswordGrantClient in CommonUtil and ports UNIFI/waters_connect native readers + managed account authentication to use it (IdentityModel 7-compatible).
• Updates UNIFI managed session/account HTTP calls to use HttpClientWithProgress and improves WatersConnect auth error message fallback behavior.
• Fixes UnifiFunctionalTest replicate graph handling; updates VCS trigger path mappings; fixes Boost.Build MSVC assembly dependency edges and adds a Jamfile target to build pwiz.CommonUtil.dll.

File summaries

File	Description
scripts/misc/vcs_trigger_and_paths_config.py	Updates path→target mappings so WatersConnect/Unifi/RemoteApi changes trigger the intended CI targets.
pwiz_tools/Skyline/TestConnected/UnifiFunctionalTest.cs	Fixes multi-replicate chromatogram graph lookup and curve-count expectations; guards waters_connect-only test assertions.
pwiz_tools/Shared/CommonUtil/SystemUtil/OAuthPasswordGrantClient.cs	Adds a shared OAuth password-grant token request helper that returns IdentityModel TokenResponse under IdentityModel 7 constraints.
pwiz_tools/Shared/CommonUtil/CommonUtil.csproj	Adds IdentityModel reference and includes the new OAuth helper source file.
pwiz_tools/Shared/CommonMsData/RemoteApi/WatersConnect/WatersConnectAccount.cs	Uses shared OAuth helper for token requests and improves error message fallback for certain OAuth error types.
pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiSession.cs	Switches UNIFI folder/file listing to HttpClientWithProgress.SendRequest with explicit request objects.
pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiAccount.cs	Replaces IdentityModel TokenClient usage with shared OAuth helper and returns an authenticated HttpClientWithProgress.
pwiz_aux/msrc/utility/vendor_api/UNIFI/WatersConnectData.ipp	Ports native waters_connect token acquisition to shared OAuth helper (removes TokenClient).
pwiz_aux/msrc/utility/vendor_api/UNIFI/UnifiData.cpp	Ports native UNIFI token acquisition to shared OAuth helper (removes TokenClient / AuthenticationStyle).
pwiz_aux/msrc/utility/vendor_api/UNIFI/Jamfile.jam	Adds build target for pwiz.CommonUtil.dll via MSBuild and wires it into <assembly> dependencies.
libraries/boost-build/src/tools/msvc.jam	Ensures /FU assembly references depend on generated assembly targets to prevent compile races.

Review details

Suppressed comments (2)

pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiSession.cs:68

• The HttpResponseMessage returned by SendRequest() is IDisposable; it should be disposed to promptly release the underlying response content/connection resources. Wrap the response in a using declaration before reading Content.

            var response = httpClient.SendRequest(request);

pwiz_tools/Shared/CommonMsData/RemoteApi/Unifi/UnifiAccount.cs:160

• The HttpResponseMessage from SendRequest() should be disposed (IDisposable) to avoid holding response resources longer than necessary. Use a using declaration for the response before reading Content.

            var response = httpClient.SendRequest(request);

• Files reviewed: 11/13 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[chambm]

@rita-gwen See what you think about this. It should supersede #4632 and will make things a bit easier when merging into the .NET port I think.

[rita-gwen]

Looks good. I tested it against the existing WatersConnect server, works fine. Did you consider adding auth token caching to UNIFI same way it is done for w_c? It would save some unnecessary authentication calls.

[chambm]

Looks good. I tested it against the existing WatersConnect server, works fine. Did you consider adding auth token caching to UNIFI same way it is done for w_c? It would save some unnecessary authentication calls.

Not really, I consider UNIFI to be legacy code that won't see much maintenance going forward unless Waters or a user asks for it. Thanks for your review comments!