Skip to content

fix(pairing): clarify reconnect and startup state#616

Merged
shanselman merged 2 commits into
mainfrom
fix-pairing-observability-cleanup
Jun 22, 2026
Merged

fix(pairing): clarify reconnect and startup state#616
shanselman merged 2 commits into
mainfrom
fix-pairing-observability-cleanup

Conversation

@vincentkoc

Copy link
Copy Markdown
Member

Summary

  • register setup-engine startup opt-in through a logon-triggered scheduled task, with the existing Run-key path as fallback
  • carry operator/node credential source through connection snapshots so Connection UI can say when reconnect is using a paired device token
  • suppress placeholder 1.0.0 app-version display for offline orphan paired-node rows, while preserving matched/online versions

Validation

  • git diff --check
  • local .NET validation blocked on this mac shell: dotnet and pwsh are not installed/resolving, and ./build.ps1 is not directly executable here

Notes

@clawsweeper

clawsweeper Bot commented Jun 1, 2026

Copy link
Copy Markdown

Codex review: found issues before merge. Reviewed June 22, 2026, 6:14 PM ET / 22:14 UTC.

Summary
The branch adds credential-source fields to connection snapshots and UI labels, introduces scheduled-task-backed autostart with Run-key fallback and cleanup, and hides placeholder 1.0.0 versions for offline orphan nodes.

Reproducibility: yes. for the review findings: source inspection shows the PR queries only task existence and deletes the HKCU Run fallback after task creation. I did not run a live Windows logon smoke in this read-only review.

Review metrics: 2 noteworthy metrics.

  • Diff size: 18 files changed, +437/-28. The patch spans connection state, shared helpers, setup, tray UI, and tests, so merge review needs to cover upgrade behavior rather than one isolated function.
  • Startup mechanisms: 1 existing Run-key path plus 1 new scheduled-task path. The PR changes the persisted autostart source of truth for existing Start with Windows users.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🌊 off-meta tidepool
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:

  • Verify scheduled-task enabled state and intended user scope before reporting autostart enabled.
  • Preserve or explicitly migrate the Run-key fallback with upgrade coverage.
  • [P2] Add current AGENTS-required validation plus a Windows logon or reboot startup smoke.

Mantis proof suggestion
A Windows desktop proof would materially help because the risky behavior is task-backed startup plus visible Connection settings labeling. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis visual task: on Windows, verify Start with Windows creates/enables the OpenClaw Companion task, preserves startup after logon, and Connection settings shows paired-token labels.

Risk before merge

  • [P2] Merging can make existing Start with Windows users lose their working HKCU Run fallback if schtasks /Create succeeds but the scheduled task is disabled, wrong-scope, blocked, or does not launch at the next logon.
  • [P1] The settings UI can report autostart enabled when the scheduled task merely exists, because the PR does not verify the task enabled state or intended user scope.
  • [P1] Manual cleanup docs still mention only the legacy Run-key path, so users may leave the new OpenClaw Companion scheduled task behind after uninstall or troubleshooting.
  • [P1] The PR body still does not report the AGENTS-required build/shared/tray validation or a Windows logon/reboot startup smoke for the current head.

Maintainer options:

  1. Harden task-backed startup before merge (recommended)
    Preserve the existing Run-key fallback or add a tested migration until the scheduled task is proven enabled, per-user as intended, and able to launch after logon, then update cleanup docs and proof.
  2. Split the low-risk reconnect UI cleanup
    Maintainers can separate credential-source labels and orphan-version suppression from the startup persistence migration if they want the observability cleanup without accepting autostart upgrade risk yet.
  3. Accept the task cutover risk
    Maintainers may intentionally accept the scheduled-task-first behavior, but they would own upgrade and cleanup support for users whose existing Run-key startup stops working.

Next step before merge

  • [P2] Needs human review of the scheduled-task startup migration and upgrade proof; the remaining blocker is compatibility/product judgment, not a safe automated repair.

Security
Cleared: No concrete security or supply-chain regression was found; the diff adds System32 schtasks.exe invocation but no third-party code, secret handling expansion, or broader permissions.

Review findings

  • [P1] Preserve the Run-key fallback until task startup is proven — src/OpenClaw.Tray.WinUI/Services/AutoStartManager.cs:40
  • [P2] Check task enabled state before reporting autostart — src/OpenClaw.Shared/WindowsStartupTaskRegistration.cs:38-41
  • [P3] Document the scheduled-task cleanup path — src/OpenClaw.Shared/WindowsStartupTaskRegistration.cs:24-29
Review details

Best possible solution:

Land the credential-source and version-display cleanup with the startup migration only after the scheduled-task enabled/scope semantics, fallback migration, docs, and Windows startup proof are explicit.

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

Yes for the review findings: source inspection shows the PR queries only task existence and deletes the HKCU Run fallback after task creation. I did not run a live Windows logon smoke in this read-only review.

Is this the best way to solve the issue?

No, not yet. The reconnect-state display direction is useful, but the scheduled-task migration is not the narrowest maintainable solution until enabled-state, scope, fallback, docs, and Windows proof are covered.

Full review comments:

  • [P1] Preserve the Run-key fallback until task startup is proven — src/OpenClaw.Tray.WinUI/Services/AutoStartManager.cs:40
    SetAutoStart(true) deletes the existing HKCU Run value immediately after schtasks /Create returns success. Task creation does not prove the task is enabled, scoped to the intended user, or will launch at the next logon, so existing users with a working startup opt-in can silently lose autostart; keep the fallback until the task-backed path is verified or add explicit migration coverage.
    Confidence: 0.84
  • [P2] Check task enabled state before reporting autostart — src/OpenClaw.Shared/WindowsStartupTaskRegistration.cs:38-41
    Exists() only runs schtasks /Query, so a disabled OpenClaw Companion task still counts as enabled in IsAutoStartEnabled(). Please query or parse the actual enabled state and expected user scope before using the task as an active autostart signal.
    Confidence: 0.82
  • [P3] Document the scheduled-task cleanup path — src/OpenClaw.Shared/WindowsStartupTaskRegistration.cs:24-29
    Adding the OpenClaw Companion scheduled task creates a new persistent autostart artifact, but the current uninstall docs still describe only Run-key cleanup. Update the cleanup/troubleshooting guidance so users can remove both the task and legacy registry entry.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against a346b9481958.

Label changes

Label changes:

  • add rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🧂 unranked krab, so this older rating label is no longer current.

Label justifications:

  • P2: This is a normal-priority connection/startup correctness improvement with real but bounded upgrade impact.
  • merge-risk: 🚨 compatibility: The PR can change or break existing autostart behavior by replacing a stored HKCU Run opt-in with a scheduled task during upgrade.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🌊 off-meta tidepool and patch quality is 🧂 unranked krab.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The external-contributor proof gate does not apply to this repository-member PR, though Windows startup proof remains a merge-readiness risk above.
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.
  • [P1] Windows logon or reboot smoke verifying the scheduled task launches the tray and existing autostart opt-ins are preserved.

What I checked:

  • Repository policy read and applied: AGENTS.md was read fully; its connection/startup architecture guidance and validation requirements apply because the PR touches pairing, node reconnect, setup, tray UI, and autostart behavior. (AGENTS.md:1, a346b9481958)
  • Architecture docs inspected: The connection architecture docs make GatewayConnectionManager the owner of operator/node state and call out credential precedence, which supports the credential-source part of the review. (docs/CONNECTION_ARCHITECTURE.md:1, a346b9481958)
  • Maintainer discussion remains relevant: The discussion asks for scheduled-task scope/enabled-state clarification and upgrade/missing-task behavior before merge; the latest head still queries only task existence and removes the Run-key fallback after task creation. (12a84b80884a)
  • Task existence is treated as enabled autostart: On the PR head, Exists() only runs schtasks /Query /TN OpenClaw Companion, so a disabled or wrong-scope task can still make the UI report autostart enabled. (src/OpenClaw.Shared/WindowsStartupTaskRegistration.cs:38, 12a84b80884a)
  • Run-key fallback is removed after task creation: On the PR head, SetAutoStart(true) deletes the HKCU Run value immediately after scheduled-task registration succeeds, which can break an existing startup opt-in if the new task does not actually launch at logon. (src/OpenClaw.Tray.WinUI/Services/AutoStartManager.cs:38, 12a84b80884a)
  • Current cleanup docs are Run-key-only: Current uninstall docs still tell users to remove only HKCU\...\Run\OpenClawTray, so the new scheduled-task artifact would be missing from manual cleanup guidance. (docs/uninstall-msix.md:58, a346b9481958)

Likely related people:

  • vincentkoc: Prior merged work added persisted node-token reconnect and companion app-version reporting that this PR follows up. (role: recent area contributor; confidence: high; commits: ee8fd4ef4ff0, 66683a33e770, 64b650cb3313; files: src/OpenClaw.Connection/GatewayConnectionManager.cs, src/OpenClaw.Shared/WindowsNodeClient.cs, src/OpenClaw.Shared/AppVersionInfo.cs)
  • shanselman: Current-main blame shows recent baseline ownership of AutoStartManager and InstanceMerger, and the latest PR head commit also touches the scheduled-task timeout and async autostart path. (role: recent startup and shared-state contributor; confidence: high; commits: d23f8ca50013, 12a84b80884a; files: src/OpenClaw.Tray.WinUI/Services/AutoStartManager.cs, src/OpenClaw.Shared/InstanceMerger.cs, src/OpenClaw.Shared/WindowsStartupTaskRegistration.cs)
  • ranjeshj: Recent current-main history hardened gateway connection and pairing flows near credential resolution and connection state paths affected by the PR. (role: recent connection and credential-flow contributor; confidence: high; commits: ea36b12f9e4c, e3c6504aaf27, 2fcfe76abcbf; files: src/OpenClaw.Connection/GatewayConnectionManager.cs, src/OpenClaw.Connection/CredentialResolver.cs, src/OpenClaw.Connection/GatewayRegistry.cs)
  • Jason (Json): Recent current-main work surfaces node reapproval state in Command Center, adjacent to this PR's pairing and connection-label UI changes. (role: adjacent node approval UI contributor; confidence: medium; commits: cb68abf8e75e; files: src/OpenClaw.Connection/GatewayConnectionManager.cs, src/OpenClaw.Tray.WinUI/Pages/ConnectionPagePlan.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. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jun 1, 2026
@vincentkoc vincentkoc force-pushed the fix-pairing-observability-cleanup branch from 6d83c03 to e365ba3 Compare June 3, 2026 05:14
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. and removed rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jun 3, 2026
@vincentkoc vincentkoc marked this pull request as ready for review June 3, 2026 05:28
@clawsweeper clawsweeper Bot added status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 3, 2026
@shanselman

Copy link
Copy Markdown
Contributor

Thanks for the cleanup work here. I don't think this is safe to merge yet because the startup/scheduled-task behavior still needs author clarification and hardening before we can be confident it won't misreport or change autostart state incorrectly.

Could you please address the scheduled-task scope/enabled-state concerns directly — especially whether the task is per-user vs all-user, how the UI determines the true enabled state, and how reconnect/startup status behaves after upgrades or missing tasks? Once that's explicit in code/tests/proof, this can get another focused review.

@shanselman

Copy link
Copy Markdown
Contributor

Maintainer note: thanks for this — the direction is useful, but the branch is currently conflicting in CompletePage.xaml.cs, ConnectionPage.xaml.cs, and ConnectionPageApproveCommandTests.cs. Because this touches connection/startup state and autostart behavior, I don't want to do a quick maintainer-side conflict resolution and accidentally change behavior. Please rebase on current main and keep the reconnect-state changes tightly scoped; happy to re-review after that.

vincentkoc and others added 2 commits June 22, 2026 14:24
Preserve the operator credential source when reconnecting the Windows node without replacing an existing same-gateway operator connection. Move the scheduled-task auto-start work off the WinUI thread and terminate schtasks.exe if it times out.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@shanselman shanselman force-pushed the fix-pairing-observability-cleanup branch from e365ba3 to 12a84b8 Compare June 22, 2026 22:03
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. labels Jun 22, 2026
@shanselman shanselman merged commit 260fb90 into main Jun 22, 2026
20 checks passed
@shanselman shanselman deleted the fix-pairing-observability-cleanup branch June 22, 2026 22:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. 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.

2 participants