Add pre-release deep repo review report (2026-07-07)

[BrettKinny]

Full read-through of production source, build, and test run ahead of the
next major release. Documents 6 correctness findings (thread-marshalling
inconsistency in config load, mixed UTC/local CSV timestamps, scope
plotting the truncated display value, invisible --connect warning, UTC
timestamp display, dead scope key mappings), robustness notes, tech debt
(Terminal.Gui CS0618 set), and confirms security/release-engineering
posture. 696/696 tests passing.

Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01PD9QJLoPtFsT3JTcKtB4j4

[claude]

Review

This PR is unusual in a good way: it's a self-review report (docs/deep-repo-review-2026-07-07.md) plus a follow-up commit that fixes the 4 highest-severity findings from that same report. I read through both commits in full; here's my assessment.

Correctness — the fixes look solid

UI-thread marshalling (MainWindow.cs)

• The UiThread.RunAsync<T> helper (Utilities/UiThread.cs) is well built: TaskCreationOptions.RunContinuationsAsynchronously avoids re-entrant continuation execution on the Application.Invoke callback, and exceptions from func() are correctly funneled into tcs.SetException rather than being swallowed.
• Wrapping the password-prompt (Application.Run(pwDialog)) in RunAsync and capturing (Confirmed, Password) before the using disposes the dialog is correct — the tuple is materialized before the lambda scope ends.
• The rest of LoadConfigurationAsync/SaveConfigurationAsync now consistently wraps post-await UI work in UiThread.Run, matching the existing convention already used in ConnectAsync/ReconnectAsync/DisconnectAsync. Good catch turning an inconsistency into a real fix rather than just flagging it.
• Minor gap: there's no direct unit test for UiThread.RunAsync itself (only indirect coverage via the config-load path, which per the repo's own "Common Issues" note may not exercise the OPC UA server in CI). Given it's a small, self-contained helper, that's a low-severity gap, but worth a lightweight test if there's an easy way to fake Application.Invoke.

Scope RawValue plotting (ScopeView.cs)

• TryGetSample correctly prefers RawValue and falls back to Value when RawValue is empty (verified against SubscriptionManager.ProcessValueChange, which always sets both together, including for the "(pending)"/"(reconnecting...)" placeholders — so the (...) rejection in TryParseValue still correctly filters those out via RawValue too).
• Boolean plotting as 0/1: FormatRawValue routes bool through the IFormattable branch, producing "True"/"False", which bool.TryParse in TryParseValue handles correctly. The ordering (bool check before float parse) is correct and doesn't risk misclassifying numeric strings.
• Parsing RawValue with NumberStyles.Float + InvariantCulture is correct since FormatRawValue always uses invariant formatting.
• New test file (ScopeViewSampleTests.cs) covers the sub-cent-amplitude regression, the display-value fallback, booleans, and non-numeric rejection — good coverage of the actual bug being fixed.

CSV UTC normalization (CsvRecordingManager.cs)

• item.Timestamp ?? DateTime.UtcNow correctly relies on MonitoredNode.Timestamp being DateTime? (defaults to null, not DateTime.MinValue), so the fallback path only triggers when no OPC UA timestamp was ever recorded — confirmed this isn't accidentally short-circuited.
• Treating Kind=Unspecified as UTC (rather than converting) matches the OPC UA SourceTimestamp convention and is explicitly tested (RecordValue_UsesIso8601TimestampFormat), while Kind=Local is explicitly converted and separately tested (RecordValue_LocalKindTimestamp_IsConvertedToUtc). Good, this is exactly the ambiguous case the original bug report called out.

--connect warning ordering (Program.cs)

• Moving the warning before Application.Init() and gating it on string.IsNullOrEmpty(configPath) preserves the existing "config file takes precedence" behavior while fixing the visibility bug. Confirmed the removed else if branch inside the mainWindow try-block had no other side effects.

Minor/non-blocking observations

• The report doc (docs/deep-repo-review-2026-07-07.md) is a dated point-in-time artifact. It's a reasonable record of the review, but consider whether recurring reviews like this belong in docs/ long-term vs. living only in the PR description/history — otherwise docs/ may accumulate similar dated snapshots that go stale. Not a blocker, just a process question.
• Findings 5 and 6 from the report (local-time display for monitored-variable timestamps, dead shifted-digit key mappings in ScopeView.OnKeyDown) and the robustness notes (session-swap race, stuck "(reconnecting...)" rows, etc.) are correctly left unfixed here and called out as follow-ups — good scoping, this PR doesn't try to do everything at once.
• I wasn't able to execute dotnet build/dotnet test in this review environment to independently confirm the claimed 710/710, so that number is taken on faith from the PR description; the manual trace above didn't turn up anything that would fail compilation or the new/updated tests' assertions.

Overall

No bugs found in the fixes themselves — they're precise, address the exact root causes described, and each one comes with a regression test. Nice example of using a review pass to drive immediately-actionable, well-scoped fixes rather than a big-bang change.