Skip to content

fix(chat): reset keyless diagnostic on reconnect#646

Merged
shanselman merged 1 commit into
masterfrom
ranjeshj/surface-keyless-diagnostics
Jun 7, 2026
Merged

fix(chat): reset keyless diagnostic on reconnect#646
shanselman merged 1 commit into
masterfrom
ranjeshj/surface-keyless-diagnostics

Conversation

@ranjeshj

@ranjeshj ranjeshj commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fixes Surface visible diagnostics for keyless chat events #458 by resetting the one-shot keyless chat/agent event diagnostic when the chat provider reconnects.
  • Keeps the existing safety behavior: keyless inbound events are still dropped instead of routed into a synthetic main timeline.
  • Adds a regression test that verifies a keyless event emits the visible diagnostic again after disconnect + reconnect, without exposing dropped payload content.

Triage

Fix size: small, confidence >=95%. The scoped gap was that _keylessEventDiagnosticRaised survived reconnects, so users could lose the visible warning if a still-broken gateway emitted keyless events after reconnect.

Validation

With OPENCLAW_REPO_ROOT set to this worktree via short path C:\ocw458:

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

Note: initial full build from the generated long worktree path failed in WinAppSDK PRI generation (CommunityToolkit.WinUI.Controls.SettingsControls.pri.xml missing). Rerunning from a short junction path avoided that environment/path issue and passed.

Reset the one-shot keyless chat/agent event diagnostic when the chat provider reconnects, so a still-broken gateway produces a visible warning again after reconnect instead of going silent. Add a regression test covering disconnect + reconnect notification behavior without exposing dropped payload content.

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

clawsweeper Bot commented Jun 2, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed June 2, 2026, 7:16 PM ET / 23:16 UTC.

Summary
The PR resets the keyless chat/agent diagnostic flag on reconnect and adds a regression test for repeated visible diagnostics after reconnect.

Reproducibility: yes. from source: current master keeps the one-shot diagnostic flag set after the first keyless event, and the reconnect path does not reset it. A later keyless event after reconnect would therefore not raise the visible warning again.

Review metrics: 1 noteworthy metric.

  • Patch size: 2 files changed, 27 additions, 0 deletions. The change is tightly scoped to one reconnect cleanup path and one regression test.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🌊 off-meta tidepool
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

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

Rank-up moves:

  • none.

Risk before merge

  • [P1] This read-only review did not independently rerun the artifact-generating build/test validation; it relies on source inspection and the PR body's reported validation.

Maintainer options:

  1. Decide the mitigation before merge
    Land the focused reconnect reset with its regression test after maintainer review, preserving the existing safety behavior of dropping keyless events instead of routing them into a synthetic timeline.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P2] The PR is collaborator-authored and appears correct, so the remaining action is maintainer review/merge rather than cleanup close or automated repair.

Security
Cleared: No security or supply-chain concern was found in the small application-code and test-only diff.

Review details

Best possible solution:

Land the focused reconnect reset with its regression test after maintainer review, preserving the existing safety behavior of dropping keyless events instead of routing them into a synthetic timeline.

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

Yes from source: current master keeps the one-shot diagnostic flag set after the first keyless event, and the reconnect path does not reset it. A later keyless event after reconnect would therefore not raise the visible warning again.

Is this the best way to solve the issue?

Yes; resetting the flag inside the existing justReconnected cleanup path is the narrow maintainable fix and keeps the current drop-rather-than-route safety property intact.

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P2: The PR addresses a normal-priority diagnostic bug where users can miss a visible warning after reconnect, with limited blast radius.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The real-behavior proof gate is not applied here because the PR is collaborator-authored; the PR body reports build and test validation.

Label justifications:

  • P2: The PR addresses a normal-priority diagnostic bug where users can miss a visible warning after reconnect, with limited blast radius.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🌊 off-meta tidepool and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Not applicable: The real-behavior proof gate is not applied here because the PR is collaborator-authored; the PR body reports build and test validation.
Evidence reviewed

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:

Likely related people:

  • Régis Brid: The available local history and blame attribute the relevant chat provider reconnect and keyless diagnostic code to the v0.6.0 chat hardening commit. (role: recent area contributor; confidence: medium; commits: 7d9152f427a3; files: src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs, tests/OpenClaw.Tray.Tests/OpenClawChatDataProviderTests.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.

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. P2 Normal priority bug or improvement with limited blast radius. labels Jun 2, 2026
@shanselman shanselman merged commit 3d26594 into master Jun 7, 2026
24 checks passed
@shanselman shanselman deleted the ranjeshj/surface-keyless-diagnostics branch June 7, 2026 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Surface visible diagnostics for keyless chat events

2 participants