Skip to content

Log previously swallowed exceptions#671

Open
christineyan4 wants to merge 3 commits into
openclaw:masterfrom
christineyan4:fix-swallowed-exceptions
Open

Log previously swallowed exceptions#671
christineyan4 wants to merge 3 commits into
openclaw:masterfrom
christineyan4:fix-swallowed-exceptions

Conversation

@christineyan4
Copy link
Copy Markdown
Contributor

@christineyan4 christineyan4 commented Jun 3, 2026

Summary

  • Log exceptions that were previously swallowed in tray/setup flows.
  • Add recent sanitized tray log tail to the diagnostics/debug bundle.
  • Preserve safe user-facing error text while writing detailed exception information to openclaw-tray.log.
  • Sanitize sensitive tray log details before writing to tray text logs, diagnostics JSONL, and crash logs; the diagnostics bundle re-sanitizes as defense in depth.

Validation

  • Ran ./build.ps1 successfully.
  • Ran dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore successfully.
  • Ran dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore successfully.
  • Ran manual tray/gateway smoke test with local gateway on ws://127.0.0.1:19001/.
  • Verified tray connected to gateway and node came online.
  • Triggered known invalid gateway failure with ws://127.0.0.1:9/.
  • Confirmed UI showed safe generic failure text while detailed connection-refused errors were logged to %LOCALAPPDATA%\OpenClawTray\openclaw-tray.log.

Proof notes

Invalid gateway failure generated log lines like:

Gateway connectivity probe failed for http://127.0.0.1:9: No connection could be made because the target machine actively refused it. (127.0.0.1:9)
Gateway /health connectivity probe failed for http://127.0.0.1:9: No connection could be made because the target machine actively refused it. (127.0.0.1:9)

Diagnostics bundle now includes a bounded sanitized Recent Tray Log section sourced from the same tray log.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 3, 2026

Codex review: needs changes before merge. Reviewed June 3, 2026, 12:53 PM ET / 16:53 UTC.

Summary
The PR logs previously swallowed tray/setup exceptions and adds a bounded Recent Tray Log tail to the diagnostics/debug bundle.

Reproducibility: yes. for the patch defect from source inspection: a tray log line containing an email, private IP, or internal host passes through RedactSupportLogLine with token/path redaction only before being appended to the shareable debug bundle. I did not need to run the app to establish that source-level path.

Review metrics: 2 noteworthy metrics.

  • Merge diff surface: 18 files changed, +345/-60. The PR touches broad tray/setup logging plus a diagnostic export path, so privacy and validation review matter before merge.
  • Shareable diagnostic surfaces: 1 new log-tail section added. The new section is copied or saved by users and needs the same privacy treatment as existing support diagnostics.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦞 diamond lobster ✨ media proof bonus
Patch quality: 🧂 unranked krab
Result: blocked by patch quality or review findings.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P2] Apply full support redaction to the new log-tail bundle path and add focused redaction tests.
  • [P2] Report the required ./build.ps1, shared test, and tray test validation after the fix.

Risk before merge

  • [P1] Merging as-is would let the debug bundle include recent tray log lines after only token/path redaction, while ordinary private hosts, IP addresses, emails, and message-like values can remain visible in a bundle users are encouraged to copy or save.
  • [P1] The PR body does not report the AGENTS.md-required shared and tray test commands, so validation is incomplete for this branch.

Maintainer options:

  1. Tighten log-tail redaction before merge (recommended)
    Apply the existing support-bundle host/IP/email/path/token redaction to every Recent Tray Log line and cover it with focused tray tests using representative log text.
  2. Pause the log-tail export
    If maintainers are not ready to define the privacy contract for exporting recent logs, leave the exception logging changes and remove the bundle log-tail section for a narrower PR.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Apply full support-bundle redaction to the new Recent Tray Log tail before it is copied or saved: make RedactSupportLogLine redact tokens, local paths, URLs/hosts/IPs/emails/user@host patterns consistently with RedactSupportValue, add focused tray tests with representative log lines, and run ./build.ps1 plus the shared and tray test commands from AGENTS.md.

Next step before merge

  • [P2] A narrow repair can preserve the useful logging changes while tightening the new debug-bundle redaction path and adding focused tests.

Security
Needs attention: The diff introduces a concrete privacy/security-boundary concern by exporting recent tray logs with weaker redaction than existing support diagnostics.

Review findings

  • [P1] Redact log-tail diagnostics before sharing — src/OpenClaw.Tray.WinUI/Helpers/CommandCenterTextHelper.cs:384-397
Review details

Best possible solution:

Keep the logging improvements, but make the new log-tail bundle path use the full support redaction policy and add focused redaction tests before merge.

Do we have a high-confidence way to reproduce the issue?

Yes for the patch defect from source inspection: a tray log line containing an email, private IP, or internal host passes through RedactSupportLogLine with token/path redaction only before being appended to the shareable debug bundle. I did not need to run the app to establish that source-level path.

Is this the best way to solve the issue?

No: the overall direction is useful, but the new diagnostic export should reuse or extend the existing support redaction policy before merge. A narrow fix in the helper plus focused contract tests is the maintainable path.

Full review comments:

  • [P1] Redact log-tail diagnostics before sharing — src/OpenClaw.Tray.WinUI/Helpers/CommandCenterTextHelper.cs:384-397
    BuildDebugBundle now copies recent tray log lines into the diagnostics bundle, but RedactSupportLogLine only runs TokenSanitizer plus local-path replacement. Ordinary private values that the existing support bundle redacts, such as emails, IPs, internal hosts, and user@host strings, can therefore be copied/saved from the new Recent Tray Log section. Please apply the full support redaction policy to each log line before appending it and add focused tests for those cases.
    Confidence: 0.9

Overall correctness: patch is incorrect
Overall confidence: 0.88

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against f839cf53aa0c.

Label changes

Label changes:

  • add P2: This is a normal-priority diagnostics/logging improvement with a concrete pre-merge privacy blocker and limited blast radius.
  • add merge-risk: 🚨 security-boundary: The PR adds recent tray logs to a user-shareable diagnostics bundle without applying the existing host/IP/email support redaction.
  • add proof: 📸 screenshot: Contributor real behavior proof includes screenshot evidence. The PR body and screenshots show an after-fix tray/gateway smoke, invalid gateway failure logs, and the diagnostics dialog containing the new Recent Tray Log section.
  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦞 diamond lobster and patch quality is 🧂 unranked krab.
  • remove rating: 🦪 silver shellfish: Current PR rating is rating: 🧂 unranked krab, so this older rating label is no longer current.

Label justifications:

  • P2: This is a normal-priority diagnostics/logging improvement with a concrete pre-merge privacy blocker and limited blast radius.
  • merge-risk: 🚨 security-boundary: The PR adds recent tray logs to a user-shareable diagnostics bundle without applying the existing host/IP/email support redaction.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦞 diamond lobster and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (screenshot): The PR body and screenshots show an after-fix tray/gateway smoke, invalid gateway failure logs, and the diagnostics dialog containing the new Recent Tray Log section.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body and screenshots show an after-fix tray/gateway smoke, invalid gateway failure logs, and the diagnostics dialog containing the new Recent Tray Log section.
  • proof: 📸 screenshot: Contributor real behavior proof includes screenshot evidence. The PR body and screenshots show an after-fix tray/gateway smoke, invalid gateway failure logs, and the diagnostics dialog containing the new Recent Tray Log section.
Evidence reviewed

Security concerns:

  • [medium] Recent tray logs can expose private diagnostics — src/OpenClaw.Tray.WinUI/Helpers/CommandCenterTextHelper.cs:384
    The new bundle section sanitizes tokens and local paths but not ordinary private hosts, IP addresses, emails, or message-like log content before users copy or save the bundle.
    Confidence: 0.9

Acceptance criteria:

  • [P1] ./build.ps1.
  • [P1] dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore.
  • [P1] dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore.

What I checked:

  • PR merge diff adds a shareable log tail: The merge result adds BuildRecentTrayLogTail(Logger.LogFilePath) to the debug bundle and appends each line after RedactSupportLogLine, which currently applies token sanitization and local-path replacement only. (src/OpenClaw.Tray.WinUI/Helpers/CommandCenterTextHelper.cs:384, fe544761d1e6)
  • Existing support redaction is broader: Current main already has RedactSupportValue patterns for URL hosts, IPs, emails, user@host, and host:port strings, and the existing support/debug sections use it for topology, runtime, tunnel, and port diagnostics. (src/OpenClaw.Tray.WinUI/Helpers/CommandCenterTextHelper.cs:470, fe544761d1e6)
  • Token sanitizer does not cover ordinary identifiers: TokenSanitizer only targets bearer/secret/token-like patterns; its tests explicitly keep ordinary username/email fields unredacted, so it is not a substitute for support-bundle value redaction. (src/OpenClaw.Shared/TokenSanitizer.cs:23, f839cf53aa0c)
  • Proof inspected: The PR body reports a successful build, manual local gateway smoke, invalid gateway failure, and safe UI text; the attached screenshots show the diagnostics dialog containing the Recent Tray Log section and log lines for the invalid gateway failure.
  • Repository validation policy: AGENTS.md requires ./build.ps1, shared tests, and tray tests after changes; the PR body reports the build and manual smoke, but not the two required test commands. (AGENTS.md:5, f839cf53aa0c)
  • History and routing: Current main blames the diagnostics bundle, dialog warning, support redaction helpers, and token sanitizer to commit 7d9152f. (src/OpenClaw.Tray.WinUI/Helpers/CommandCenterTextHelper.cs:14, 7d9152f427a3)

Likely related people:

  • Régis Brid: Introduced the diagnostics bundle, support redaction helpers, diagnostics dialog warning, related tests, and token sanitizer now used by this PR. (role: recent area contributor; confidence: high; commits: 7d9152f427a3; files: src/OpenClaw.Tray.WinUI/Helpers/CommandCenterTextHelper.cs, src/OpenClaw.Tray.WinUI/Windows/DiagnosticsBundleDialog.xaml, src/OpenClaw.Shared/TokenSanitizer.cs)
  • Ranjesh: Recently changed the connection/setup surfaces this PR also touches, including gateway configuration timeout and setup-managed WSL gateway behavior. (role: recent adjacent contributor; confidence: medium; commits: 3ddade7072b0, f4d4793f744f; files: src/OpenClaw.Tray.WinUI/Pages/ConnectionPage.xaml.cs, src/OpenClaw.SetupEngine/SetupSteps.cs)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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 keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@christineyan4
Copy link
Copy Markdown
Contributor Author

Screenshot 2026-06-03 122131 image

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Jun 3, 2026
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@clawsweeper clawsweeper Bot added proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 3, 2026
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@christineyan4
Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Addressed the security-boundary finding on the latest branch head. The log-tail bundle path now calls TokenSanitizer.SanitizeLogMessage(line), which applies token redaction plus local path, URL host, IP, email, and user@host redaction before the line is copied into the diagnostics bundle. The tray text log, diagnostics JSONL, and crash log now also sanitize via SanitizeLogMessage before writing so sensitive values do not first land in local logs.

Validation completed:

  • ./build.ps1
  • dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore
  • dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 3, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@christineyan4
Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

Please target base branch master for this repository; main does not exist. The previous re-review workflow failed while trying to fetch main from openclaw/openclaw-windows-node.

The PR base is master, and the latest head is 0429f49af86f7fb77cacb14842515f80982907b4.

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 3, 2026

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

Re-review progress:

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

Labels

merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. proof: sufficient Contributor real behavior proof is sufficient. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant