Skip to content

Harden configuration restore and polish the v1 release#182

Draft
BrettKinny wants to merge 1 commit into
mainfrom
agent/v1-release-review
Draft

Harden configuration restore and polish the v1 release#182
BrettKinny wants to merge 1 commit into
mainfrom
agent/v1-release-review

Conversation

@BrettKinny

Copy link
Copy Markdown
Collaborator

Summary

This follows up the v1.0.0 release review with the correctness fixes, permanent
regression coverage, and documentation polish identified during the audit.

Why

OPC UA numeric namespace indexes are allocated per session, but saved monitored
nodes relied on those indexes alone. A server restart or configuration change
could therefore make a saved node resolve incorrectly. The CLI also exposed
--connect without implementing the connection, the project had no terminal-free
--version command, and parts of the README had drifted from the pinned toolchain
and actual keyboard behavior.

What changed

  • Persist stable namespace URIs for non-standard monitored nodes and resolve them
    against the active session, while retaining backward compatibility with existing
    numeric-only configuration files.
  • Implement direct --connect startup and --version, share version formatting
    with the About dialog, and avoid printing fatal stack traces to users.
  • Add published-binary PTY regressions for issue Status bar shows "Not Connected" after config-file connect while session is healthy #178, direct connection startup,
    and version output.
  • Remove process-global culture mutation from parallel tests.
  • Align README, contributor guidance, command documentation, installer output,
    shortcuts, certificate guidance, and the exact SDK 10.0.109 requirement.
  • Add curated v1.0.0 notes, a v1.x UAT checklist, and reproducible promotional-media
    tooling and assets that drive the normal application UI.
  • Tighten generated-file ignores and repository hygiene.

Impact

Saved configurations are safer across OPC UA namespace-table changes, command-line
startup is complete and scriptable, and the public documentation now matches the
application and build environment. Existing configuration files remain supported.

Validation

  • dotnet test Opcilloscope.sln --configuration Release --no-restore — 833 passed
  • dotnet test Tests/Opcilloscope.E2ETests/Opcilloscope.E2ETests.csproj --configuration Release --no-restore — 13 passed against a fresh published binary over real PTYs
  • dotnet format Opcilloscope.sln --verify-no-changes --no-restore
  • bash -n install.sh uninstall.sh scripts/capture-media.sh
  • git diff --cached --check
  • scripts/verify-third-party-inventory.sh

@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Review: Harden configuration restore and polish the v1 release

Overall this is a well-scoped, well-tested follow-up. The namespace-URI fix addresses a real correctness gap, --connect/--version close out documented CLI gaps, and the culture-mutation fix removes a real source of test flakiness. Nice work matching the PR description to actual validation (833 tests + E2E + format/inventory checks).

Correctness

  • Namespace URI resolution (ConfigurationService.ResolveNodeId / CaptureCurrentState) — the core fix looks right: URIs take precedence over the numeric index, index 0 (standard namespace) is correctly never persisted with a URI, and legacy numeric-only configs keep working (covered by ResolveNodeId_WithoutNamespaceUri_PreservesLegacyNumericIndex). Good test coverage on the missing-URI failure path too (ResolveNodeId_WithMissingNamespaceUri_ThrowsClearError).
  • MainWindow.LoadConfigurationCoreAsync — the new _connectionManager.Client.Session?.NamespaceUris ?? throw ... is inside the existing per-node try/catch, so a missing namespace table or an unresolvable URI degrades to a per-node "failed to subscribe" warning rather than aborting the whole restore. Good.
  • SaveConfigurationCoreAsync passes _connectionManager.Client.Session?.NamespaceUris, which is null while disconnected. That's fine in practice because monitoredVariables comes from _connectionManager.SubscriptionManager?.MonitoredVariables, and SubscriptionManager should only be non-null while a session exists — just flagging the implicit coupling in case those two lifecycles ever get decoupled in a future change (worth a comment or an assertion if you want to make the invariant explicit).
  • --connect / --version CLI wiringConnectFromCommandLine reuses the existing ConnectAsync path (same as the Connect dialog), which is good reuse rather than a parallel code path. The 100ms AddTimeout before firing off the connect mirrors the existing LoadConfigFromCommandLine pattern, so it's consistent, though it's still a fixed-delay heuristic for "main loop has started" rather than an event — pre-existing pattern, not introduced by this PR, but the same fragility now applies to the new call site too.
  • Removing Console.Error.WriteLine(ex.StackTrace) for fatal errors is a reasonable UX call (stated intent in the PR body), but it does mean a truly unexpected crash now surfaces only ex.Message with no trace anywhere (nothing writes it to the log file either, since this exception path is outside the app's normal lifecycle in Main's outer catch). Not a blocker, just worth confirming that's an acceptable debugging tradeoff for a v1.x release.

Code quality

  • VersionInfo.cs cleanly extracts the previously-duplicated GetDisplayVersion logic out of MainWindow and shares it with Program.cs — good DRY cleanup, matches the "share version formatting with the About dialog" goal.
  • The WithCulture test-helper fix (removing CultureInfo.DefaultThreadCurrentCulture mutation) is a solid fix for parallel-test correctness — good catch, and the comments explain why clearly.
  • Casing cleanup (Opcilloscopeopcilloscope in installer/CLI output) is consistent with the CLAUDE.md naming convention table.

Test coverage

  • New E2E coverage (ConnectionStartupTests.cs, Version_PrintsVersionWithoutOpeningTheTui) exercises the published binary over a real PTY, matching the project's testing philosophy in docs/TESTING.md.
  • Minor: ReservePort() (bind to port 0, read it back, close, then hand the port to the OPC UA test server) has an inherent TOCTOU race — another process could grab the port between listener.Stop() and the server binding. Low risk in CI, and not a new pattern class for this codebase, but worth knowing if ConnectionStartupTests becomes flaky under parallel test load.
  • ConfigFileConnection_RemainsConnectedAfterStartupBannerCompletes uses a fixed Task.Delay(5s) to confirm the connection doesn't regress after the startup banner (issue Status bar shows "Not Connected" after config-file connect while session is healthy #178) — functional but adds real wall-clock time to the suite; an event/poll-based wait would be more robust if the banner's dismissal is observable some other way, though a fixed delay is a reasonable regression guard for a timing-related bug.

Minor/non-blocking

  • docs/media/*.gif and *.webp add several binary assets directly to the repo. Not a problem for a promotional-media use case, but worth confirming the team is fine with binary assets living in git history long-term (vs. e.g. release-attached assets) since GIFs/WebPs don't diff well and can grow clone size over time.
  • scripts/capture-media.sh interpolates $binary/$config paths into tmux new-session … "exec '$binary' '$config'" strings. These are locally-derived (mktemp/repo paths) so injection risk is minimal, but worth keeping in mind if this script is ever extended to take less-trusted input.

Security

Nothing concerning — --insecure semantics are unchanged, no new attack surface in the namespace resolution path (URIs are compared/looked-up, not used to construct code paths), and removing the stack-trace print slightly reduces the info exposed to a terminal/log-shared user.

Nice PR — the fixes are targeted, well-tested, and the docs/CLAUDE.md updates track the behavior changes accurately.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant