Code review updates - #6
Merged
Merged
Conversation
5 tasks
marcschier
added a commit
that referenced
this pull request
Jun 2, 2026
Address 7 of the 16 unresolved review comments from round 3: - #6 Tools/Opc.Ua.SourceGeneration/ModelCompilation.cs:300 - add `using System.Collections.Generic` and `using Opc.Ua.SourceGeneration.Dependency` so the file no longer spells fully-qualified type names inline. Same sweep for the `System.Collections.Generic.`/`Opc.Ua.SourceGeneration.Dependency.` prefixes at other call sites in the file. - #7 Tools/Opc.Ua.SourceGeneration.Core/Generators.cs:634 (and 3 other sites in FluentBuilderGenerator + GeneratorOptions + ModelCompilationOptions) - drop `FB-3 phase 3` / phase / track references from comments and xmldoc; describe the feature, not the rollout. - #9 Libraries/Opc.Ua.Di.Client/Hosting/OpcUaClientDiBuilderExtensions.cs:69 - re-wrap the long `<item><description>...</description></item>` doc list to fit inside the 140-char editorconfig limit. - #12 Strip `global::` qualification from all hand-written PR code (Libraries/Opc.Ua.Di.*, Applications/PumpDeviceIntegrationServer). Only kept where required to disambiguate (test-side `Pumps.PumpNodeManager` / `Pumps.PumpNodeManagerFactory` collide with the source-generated `Opc.Ua.Pumps` namespace; restored `global::` on those refs). Hand-written code now matches the .editorconfig convention of letting `using` directives carry the namespace burden; source-generated code still uses `global::` (unchanged). - #15 Remove `// -----` / `// =====` ASCII-banner `#region`-style comments from SoftwareUpdateClient.StateMachine.cs, SoftwareUpdateClient.Upload.cs, SoftwareUpdateFacetWiring.cs and SoftwareUpdateFileTransferManager.cs. Inline section comments remain unchanged. - #16 Expand 344 one-line `/// <summary>text</summary>` declarations across 72 files in Applications/, Libraries/, Stack/, Tests/ and Tools/ into the three-line `/// <summary>\n/// text\n/// </summary>` form. - Carry-over from FB-3 phase 3: while sweeping `global::` also un-qualify the hand-written Pumps server callsites (Pump #1 wiring in PumpNodeManager.cs / PumpNodeManager.Configure.cs) so the new code matches the reviewer's convention. Validation: - `dotnet build UA.slnx` — clean (0 errors, 15 unrelated warnings reported transiently by the build engine). - `Opc.Ua.SourceGeneration.Core.Tests` — 3535 passed / 8 skipped. - `Opc.Ua.Di.Tests` (Pump + DI client + SoftwareUpdate filter) — 85/85 passed. The remaining 9 round-3 items (architecture relocation #8, `=>`→`{}` body sweep #10, ObjectType-proxy refactor #11/#14, brace-formatting sweep #13, pump server Configure refactor #1/#2, DeviceHealth + functional-group exposure #3/#5 plus DI doc disambiguation #4) are tracked separately and will land in subsequent commits.
marcschier
added a commit
that referenced
this pull request
Jun 5, 2026
- Move all channel-related types into `Stack/Opc.Ua.Core/Stack/Client/Channels/` subfolder (#13). - Un-obsolete `SessionReconnectHandler` and the SessionExtensions `ReconnectAsync(connection, ct)` / `(channel, ct)` extensions; remove all SRH-related `#pragma warning disable CS0618` suppressions from applications and tests. AttachChannel/DetachChannel remain obsolete (#1, #4). - Remove the shared-budget section from MigrationGuide.md (#3). - Mark `ManagedSession.AttachChannel`/`DetachChannel` `[Obsolete]` so the warning surfaces all the way up the supported API surface (#9). - Break the `<see cref=…>` line in `Session.ChannelManager.cs` to stay under 140 chars (#10). - Move `ChannelManagerSessionFactory` from `Opc.Ua.Gds.Client.Common` into `Opc.Ua.Client` as a public, documented session-factory option (#11). - Audit other client classes for `IClientChannelManager`-aware overloads; add `ISessionFactory`-accepting overloads on `GlobalDiscoveryServerClient`, `LocalDiscoveryServerClient`, `ServerPushConfigurationClient` so any session factory (including `ChannelManagerSessionFactory`) works (#12). - Refactor MCP server (`OpcUaSessionManager`, `Program`) to use `ManagedSession` + DI-resolved `IClientChannelManager`; remove `SessionReconnectHandler` and manual keep-alive reconnect (#2). - ConnectionStateMachine code-style fixes: use named delegate types, collapse multi-line callback properties to single lines, swap `timeProvider`/`maxTotalReconnectTime` constructor param order, fix multi-line declaration (#6, #7, #8). - HTTPS resilience: confirmed `Microsoft.Extensions.Http` and `Microsoft.Extensions.Http.Resilience` 10.6.0 support `net472`/`net48`/`netstandard2.1`; removed `#if NET8_0_OR_GREATER` gating from `ManagedSessionBuilder` and `OpcUaClientBuilderExtensions` and HTTPS csproj package refs (#5). Verification: - `Opc.Ua.Client` + `Opc.Ua.Core` + `Opc.Ua.Gds.Client.Common` build clean (0 warnings, 0 errors). - 56/56 core channel-manager / retry-budget / HTTPS factory tests pass. - 129/129 client (ManagedSession + legacy SRH back-compat + SessionExtensions) tests pass.
marcschier
added a commit
that referenced
this pull request
Jul 10, 2026
…store Investigates the X509-only CTT re-run (Security User X509, 17 errors) taken after the auth-lockout fix. Server fix (CTT 018): a user-token signature built with a wrong algorithm returned the channel-level BadSecurityChecksFailed. SecurityPolicies. VerifySignatureData (shared with the channel signature checks) throws BadSecurityChecksFailed for an unexpected SignatureData.Algorithm, and X509IdentityTokenHandler.VerifyAsync propagated it unchanged. Map that algorithm-mismatch BadSecurityChecksFailed to the token-level BadIdentityTokenInvalid in the user-token path (Part 4), leaving the shared channel-signature behaviour untouched. Regression test added. Reference server enhancement (ease provisioning): the remaining 16 errors are the CTT's trusted user certs not being in the server's pki/trustedUser store (the server correctly rejects untrusted user certs per Part 4). The reference server now persists every rejected X509 user certificate to a dedicated pki/rejectedUser review store (sibling of pki/trustedUser) so an operator can move the legitimate ones into the trusted-user store and re-run. Also refactors VerifyX509IdentityToken to async (removing the sync-over-async validation call). Integration test + README note added. plans/ctt-issues.md: document CTT-script defect #6 (the '// to prevent user lockout' cleanup ActivateSession lacks error suppression, so it can fail an otherwise-passing negative test - UaR.js:219) and the run6 provisioning / rejectedUser workflow, with OPC UA spec references. Validated on net48 + net10 (X509IdentityTokenHandlerTests, SecurityX509UserTests).
7 tasks
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.
Code review updates.