Ensure exceptions are logged through standard logging mechanism#645
Open
RBrid wants to merge 6 commits into
Open
Ensure exceptions are logged through standard logging mechanism#645RBrid wants to merge 6 commits into
RBrid wants to merge 6 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Audit and fix silent exception swallows across the OpenClaw Windows Companion app. Bare `catch (Exception) { }` and similar patterns now log via IOpenClawLogger, OpenClawTray Services.Logger, or System.Diagnostics.Trace (for self-referential/static contexts) before swallowing or rethrowing.
Patterns applied:
- Typed catches with appropriate severity (Error for real bugs, Warn for user-visible degradation, Debug for routine teardown/shutdown/cleanup/dispatcher breadcrumbs).
- Atomic-write catches (McpAuthToken, DeviceIdentity) capture the original exception, Trace, attempt cleanup with its own typed+Traced catch, then rethrow.
- OCE/ODE during shutdown narrowed to Debug to avoid noise.
- Logger.cs self-referential catches use Trace.WriteLine to avoid recursion.
- App.xaml.cs OnProcessExit/LogCrash use belt-and-suspenders Trace + Console.Error with documented `nothing-left-to-call` innermost guards.
Validated with per-project dotnet build (Tray.WinUI x64 clean), Shared.Tests (2045 passed / 29 skipped), Tray.Tests (934 passed).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Codex review: needs real behavior proof before merge. Reviewed June 4, 2026, 7:18 PM ET / 23:18 UTC. Summary Reproducibility: yes. for the blocking findings from source inspection: current main logs failed setup steps at Error, while PR head makes non-exception failures Warn-only; the IPv6 and debug-bundle paths are also source-reproducible. I did not run validation because this is a read-only review. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Keep the diagnostics cleanup direction, but preserve setup Error visibility, make support-bundle sanitizer paths fail closed, align the IPv6 implementation with its tests, then provide final-head runtime proof and AGENTS.md validation. Do we have a high-confidence way to reproduce the issue? Yes for the blocking findings from source inspection: current main logs failed setup steps at Error, while PR head makes non-exception failures Warn-only; the IPv6 and debug-bundle paths are also source-reproducible. I did not run validation because this is a read-only review. Is this the best way to solve the issue? No as written; centralizing exception detail is maintainable, but it should not downgrade failed setup outcomes or bypass the sanitizer guarantees this PR is adding. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 99efc50cbc22. Label changesLabel changes:
Label justifications:
Evidence reviewedSecurity concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
clawsweeper
Bot
added
rating: 🦪 silver shellfish
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ging1 # Conflicts: # src/OpenClaw.Tray.WinUI/Services/VoiceService.cs
clawsweeper
Bot
added
rating: 🧂 unranked krab
Combines the exception-logging effort from PR openclaw#645 (this branch) with Christine's parallel PR (commit 0429f49) into a unified solution. From both PRs (kept the stronger parts of each): * TokenSanitizer: extended detection to cover IPv6 addresses (compressed, full-form, IPv4-mapped, dual notation), email addresses, user@host, Windows user-profile paths, gateway tokens/credentials. Per-message redaction wired into the tray Logger and shared diagnostics paths. * CommandCenterTextHelper: redacts support-context paths and values for diagnostics bundles using the same patterns as TokenSanitizer. * Type-prefixed log lines reinstated across DataModelStore, SettingsPage, ChatWindow, ConnectionPage, NodeService, AppCrashLogger, DeepLinkHandler, AudioPipeline, DiagnosticsJsonlService, Logger, ThemeHelper, LocalizationHelper, OpenClawChatRoot, ChatExplorationsPanelWindow, WebSocketClientBase, WindowsNodeClient, App.xaml.cs, App.ToastActivation, and the SetupEngine pipeline. Format: "TypeName: message". Hardening applied during review iterations: * IPv6 regex uses negative lookahead (?![A-Fa-f0-9:]|\.\d) so sentence punctuation (e.g. "Server at fe80::1.") still redacts while invalid partial matches (e.g. a::ffff:192.0.2.1b) do not leak. * Bracketed-IPv6 alternative uses a structural lookahead (:: or 5+ colons) plus IPAddress.TryParse validation so non-IPv6 bracketed colon-soup is left intact rather than incorrectly redacted as <ipv6>. * Zone-id capture covers RFC 6874 unreserved chars [-A-Za-z0-9._~]+ so real interface names (br-1234, eth0.1, wlan_0) are fully consumed instead of leaking the suffix after <ipv6>. * RedactIfValidIpV6 strips %scope before IPAddress.TryParse so textual and numeric zone-ids redact consistently. * Sanitize and SanitizeLogMessage wrap every regex pass in try/catch with a 1s RegexMatchTimeout, returning a stable public sentinel TokenSanitizer.SanitizerTimeoutSentinel ("[REDACTED_SANITIZER_TIMEOUT]") on timeout so adversarial input never tears down the logging or crash pipelines. SanitizeLogMessage short-circuits if any pass yields the sentinel so subsequent passes cannot re-introduce sensitive data. * CommandCenterTextHelper.RedactSupportPath / RedactSupportValue mirror the same try/catch + sentinel pattern, and share TokenSanitizer's IpV6Pattern + RedactIfValidIpV6 via [InternalsVisibleTo] to keep the two surfaces in lockstep. * New TokenSanitizerTests cover invalid bracketed forms, partial-leak resistance, sentence-punctuation redaction, RFC 6874 zone-ids, sentinel contract, and adversarial input stability. Validation: Shared.Tests 2083 passed / 29 skipped, Tray.Tests 938 passed. Co-authored-by: Christine Yan <chrisyan@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
clawsweeper
Bot
added
the
merge-risk: 🚨 security-boundary
Collaborator
|
@RBrid please take a look at these items from clawsweeper: [P1] Non-exception setup failures would lose their existing Error-level summary and become Warn-only, which makes failed setup runs less visible in logs and console output. |
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
3 participants
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.
Audit and fix silent exception swallows across the OpenClaw Windows Companion app, and sanitize sensitive details from tray logs. Bare
catch (Exception) { }and similar patterns now log via IOpenClawLogger, OpenClawTray Services.Logger, or System.Diagnostics.Trace (for self-referential/static contexts) before swallowing or rethrowing. Log messages include type-name prefixes (e.g.DataModelStore:,SettingsPage:,ChatWindow:) for greppability.Consolidates two related efforts:
0429f49awith co-authorship preserved.Exception logging
Patterns applied:
nothing-left-to-callinnermost guards.Log sanitization (from #671)
Centralized in
OpenClaw.Shared.TokenSanitizerwith a mirror inCommandCenterTextHelperfor tray support context:eth0.1,br-1234,wlan_0) redacted to<ipv6>/<ipv4>.SanitizerTimeoutSentinelconstant so callers fail safe rather than leak.IpV6Pattern/RedactIfValidIpV6exposed viaInternalsVisibleToso the tray helper reuses the same canonical regex.Review
Iterated through six rounds of dual-model adversarial code review (Opus + Codex via the Hanselman skill). All HIGH-consensus findings addressed; final round converged on a single LOW finding both reviewers explicitly recommended accepting as the necessary trade-off for VLAN-style zone identifiers.
Validation
dotnet test OpenClaw.Shared.Tests— 2083 passed / 29 skipped (3 new sanitizer theory rows for interface name varieties).dotnet test OpenClaw.Tray.Tests— 938 passed.