Skip to content

Commit 7747e60

Browse files
ranjeshjCopilotshanselman
authored
Fix zero-state chat trap and own-node pairing auto-approve
* Fix zero-state chat trap: drop CreateThreadAsync fiction, route by canonical sessionKey Root cause: OpenClawChatDataProvider maintained a C# replica of gateway session state keyed by 'threadId' that conflated UI selection token with server-assigned sessionKey. On fresh install (zero sessions), CreateThreadAsync synthesized a ChatThread with Id='main' (literal). But the gateway's hello-ok publishes BOTH sessionDefaults.mainKey ('main' alias) AND sessionDefaults.mainSessionKey ('agent:main:main' canonical), and the parser was reading the alias. Optimistic local entries landed under 'main' while gateway echoes used 'agent:main:main' — two timelines, message orphaned, UI stuck on the welcome screen with no composer. Fix (real, not a workaround — three-model agreement, dual-model code review): Protocol layer (OpenClawGatewayClient): - TryGetHandshakeMainSessionKey now prefers canonical mainSessionKey over alias. - _mainSessionKey becomes string? (no 'main' default), with Volatile.Read/Write for cross-thread publication. - _hasHandshakeSnapshot tracks handshake completion; reset on disconnect. - Extracted ResolveEffectiveSessionKey helper — throws InvalidOperationException if no canonical key resolved. No more silent fallback to a stale literal. - HandleChatEvent/HandleAgentEvent stop substituting 'main'/'unknown' for empty sessionKey — pass through so the provider can surface the protocol gap. Contract (IChatDataProvider): - Deleted CreateThreadAsync. The gateway has no session.create RPC; pretending it does was the original sin. - Added ChatComposeTarget {SessionKey, IsReady} on ChatDataSnapshot — first-class 'where to send next' concept, distinct from 'what sessions exist'. Provider (OpenClawChatDataProvider): - Deleted CreateThreadAsync impl, ResolveMainOrFirstThreadLocked, and every ?? 'main' fallback in event handlers (5 sites). Empty SessionKey now drops the event with a warning. - BuildSnapshotLocked projects ComposeTarget from bridge state and injects a synthetic ChatThread with Id=canonicalKey when the compose key has optimistic entries but no SessionInfo yet — so SessionsUpdated later replaces it in place with no re-keying or migration. - ResolveDefaultThreadIdLocked uses SessionInfo.IsMain instead of string-equal to literal 'main'. UI (OpenClawChatRoot): - Deleted StartFirstChat and the safety-net fallback (now structurally impossible to fire). - effectiveThread = selectedThread ?? composeOnlyThread; composer visible whenever ComposeTarget.IsReady. - Friendlier zero-state suggestions ('Say hi 👋', 'What can you do?', 'Give me a quick tour of OpenClaw'). - firstSendInFlight debounce clears only when the snapshot's compose-target timeline has the optimistic user entry — not on every unrelated snapshot (presence, models, channel health). Tests: deleted 2 obsolete CreateThreadAsync tests; added 7 provider tests covering fresh-install compose target, optimistic-by-canonical-key routing, SessionsUpdated reconciliation, empty-sessionKey drop, canonical echo merge, IsMain default resolution. Added 6-case OpenClawGatewayClientSessionKeyTests exercising the ResolveEffectiveSessionKey throw directly. Validation: build OK, Shared.Tests 1782/1810 (28 skipped), Tray.Tests 1096/1096. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Auto-approve own-node pairing on operator-side broadcast + surface pending state Bug: on a fresh install where the device is already paired, the Windows node sub-pairing (a separate gateway-side table, /home/openclaw/.openclaw/nodes/ paired.json) can sit empty indefinitely. The node connects, advertises its caps/commands in the connect frame, gets into the gateway's pending list — but the gateway returns caps:[], commands:[] in node.list responses until the node-sub-pairing is approved. Result: tray menu shows the node Connected with "Capabilities (0) · Commands (0)", agent capabilities silently inert. Why the existing auto-approve didn't fire: GatewayConnectionManager's OnNodePairingStatusChanged only triggers on the node-side WindowsNodeClient.PairingStatusChanged event, which fires Pending only when the node-side connection itself is pending. With the device already paired, the node-side client sees PairingStatus.Paired and never raises Pending — the only signal for the missing node-sub-pairing is the operator-side NodePairListUpdated broadcast. Fix (3 parts): 1. GatewayConnectionManager: subscribe NodePairListUpdated on every new operator client. When a pending entry's NodeId matches our own _nodeConnector.NodeDeviceId AND we have operator.admin/pairing scope, call NodePairApproveAsync. Reuses _autoApproveInFlight CAS guard and _lastAutoApprovedRequestId dedup. After approval, restart the node connection so caps propagate. 2. InstancesPage: caution-yellow banner at top of the Nodes page when any node or device pair is pending. Subtext is contextual ("This node is connected but its capabilities won't activate…" when our own deviceId is the pending one). Button navigates to ConnectionPage where the existing per-row approve/reject UI lives. 3. ConnectionPage: Initialize() now eagerly calls RequestNodePairListAsync + RequestDevicePairListAsync, and pushes any already-loaded AppState pending lists into the banner immediately, so the user sees pending state on first navigation instead of waiting for the gateway's next broadcast. Tests: 4 new cases in NodePairAutoApproveTests cover own-node approve, other-node ignored, missing scope, and dedup-on-rebroadcast. All 228 connection tests pass; Shared 1782; Tray 1096. Live verified end-to-end: after launching the new build against a WSL gateway where the node was stuck at caps:[], the operator-side auto-approve fired and node.list now reports caps:[app, browser, camera, canvas, device, location, screen, system] and 11 commands. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address Hanselman review of auto-approve path Fixes from dual-model adversarial review of fa421a3: H1 — Spin loop on approval failure: _lastAutoApprovedRequestId was set only inside if (approved). A failed approve (transient or policy reject) meant every subsequent NodePairListUpdated re-broadcast retried the same id forever. Now always recorded in finally, with a re-check against the current generation so a reconnect-during-await doesn't overwrite the new-generation null with a stale id. Same fix applied to the pre-existing node-side path. H2 — �reak after first attempt stranded second own-node pending: on exception/rejection the next own-pending in the same snapshot was never attempted (gateway might not re-broadcast if approve frame round-tripped and was rejected mid-flight). Now continue to scan all own-pending entries, then perform a single post-approval reconnect for the last-approved id (rather than per-entry, which would race with itself). H3 — CAS guard held across Task.Delay + StartNodeConnectionAsync (5–30s on WSL cold-start), starving unrelated approvals: moved the _autoApproveInFlight clear into a inally scoped JUST to the approve RPC. Post-approve reconnect runs outside the guard. Same on both paths. M1 — Stale _lastAutoApprovedRequestId write across generations: now re-checks _generation == gen before writing so a disconnect/reconnect during the approve await doesn't blow away the new generation's clean slate. M4 — Reflection-based test event raiser: added [InternalsVisibleTo("OpenClaw.Connection.Tests")] + internal RaiseNodePairListUpdatedForTests on OpenClawGatewayClient. Test helper FireNodePairListUpdated now delegates to that instead of poking the private event backing field via reflection (which silently breaks the moment events grow explicit add/remove blocks). M3 — Eager refresh swallowed exceptions silently: ConnectionPage and InstancesPage Task.Run blocks now log via Services.Logger.Warn instead of swallowing, matching the comment's stated intent. Validation: Connection 228 / Shared 1782 / Tray 1096, all pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix node auto-approve retry dedupe Only record a node pairing request as deduped after an actual approve attempt. This keeps the operator-side fallback able to approve the same request once the operator client is ready or gains approval scope. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Scott Hanselman <scott@hanselman.com>
1 parent cd4acf0 commit 7747e60

16 files changed

Lines changed: 1004 additions & 125 deletions

src/OpenClaw.Chat/ChatModels.cs

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,30 @@ public record ChatDataSnapshot(
123123
IReadOnlyDictionary<string, ChatTimelineState> Timelines,
124124
string? DefaultThreadId,
125125
string? ConnectionStatus,
126-
string[] AvailableModels);
126+
string[] AvailableModels,
127+
ChatComposeTarget ComposeTarget);
128+
129+
/// <summary>
130+
/// Describes where the UI may send the next chat message. Distinct from
131+
/// <see cref="ChatDataSnapshot.Threads"/> because, in protocols like the
132+
/// OpenClaw gateway, there is always a canonical "main" target whose session
133+
/// row may not have been materialized yet (zero sessions on fresh install).
134+
/// The UI uses this to decide whether the composer should be enabled even
135+
/// when <see cref="ChatDataSnapshot.Threads"/> is empty.
136+
/// </summary>
137+
/// <param name="SessionKey">
138+
/// The provider-resolved canonical session key for the default send target,
139+
/// or <c>null</c> when not yet known (e.g. handshake incomplete).
140+
/// </param>
141+
/// <param name="IsReady">
142+
/// True when the provider is connected, has resolved a canonical send target,
143+
/// and accepts <see cref="IChatDataProvider.SendMessageAsync"/> calls keyed by
144+
/// <see cref="SessionKey"/>.
145+
/// </param>
146+
public sealed record ChatComposeTarget(string? SessionKey, bool IsReady)
147+
{
148+
public static ChatComposeTarget NotReady { get; } = new(null, false);
149+
}
127150

128151
public sealed class ChatDataChangedEventArgs(ChatDataSnapshot snapshot) : EventArgs
129152
{
@@ -157,7 +180,10 @@ public interface IChatDataProvider : IAsyncDisposable
157180
event EventHandler<ChatProviderNotificationEventArgs>? NotificationRequested;
158181

159182
Task<ChatDataSnapshot> LoadAsync(CancellationToken cancellationToken = default);
160-
Task<ChatThread> CreateThreadAsync(string? initialMessage = null, CancellationToken cancellationToken = default);
183+
// Note: there is intentionally no CreateThreadAsync. The gateway protocol
184+
// has no "create new session" RPC; the canonical send target is exposed via
185+
// ChatDataSnapshot.ComposeTarget and the first SendMessageAsync against it
186+
// implicitly materializes the session on the server.
161187
Task SendMessageAsync(string threadId, string message, CancellationToken cancellationToken = default);
162188
Task SendMessageAsync(string threadId, string message, CancellationToken cancellationToken, IReadOnlyList<OpenClaw.Shared.ChatAttachment>? attachments) =>
163189
SendMessageAsync(threadId, message, cancellationToken);

src/OpenClaw.Connection/GatewayConnectionManager.cs

Lines changed: 137 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -239,6 +239,22 @@ private async Task ConnectCoreAsync(string? gatewayId = null)
239239
_gatewayNeedsV2Signature = true;
240240
};
241241

242+
// Operator-side auto-approve: the node-side WindowsNodeClient
243+
// only sees its own pairing state. But when the gateway broadcasts
244+
// a node.pair.requested event, the OPERATOR client gets a fresh
245+
// NodePairListUpdated push. If our own node deviceId shows up in
246+
// that pending list and we have approval scopes, approve it
247+
// immediately — otherwise the user is stuck looking at a connected
248+
// node with empty capabilities until they manually approve.
249+
// This complements (not replaces) the node-side flow on line ~810
250+
// which still handles the case where the node knows it's pending
251+
// before any operator event lands.
252+
lifecycle.DataClient.NodePairListUpdated += (s, info) =>
253+
{
254+
if (Interlocked.Read(ref _generation) != gen) return;
255+
_ = TryOperatorAutoApproveOwnNodePairAsync(info, gen);
256+
};
257+
242258
// If we already know this gateway needs v2, tell the client upfront
243259
if (_gatewayNeedsV2Signature)
244260
lifecycle.DataClient.UseV2Signature = true;
@@ -841,17 +857,23 @@ private async void OnNodePairingStatusChanged(object? sender, PairingStatusEvent
841857
_transitionSemaphore.Release();
842858
}
843859

844-
// Auto-approve node pairing if operator has admin/pairing scope
860+
// Auto-approve node pairing if operator has admin/pairing scope.
861+
// _autoApproveInFlight is a CAS guard scoped to JUST the approve RPC —
862+
// we release it before the reconnect delay so unrelated approvals
863+
// (different requestIds) aren't starved while we wait for the gateway
864+
// and node-reconnect handshake to settle (which can take 5–30s on
865+
// first connect via WSL cold-start).
845866
if (e.Status == PairingStatus.Pending && !string.IsNullOrWhiteSpace(e.RequestId)
846867
&& e.RequestId != _lastAutoApprovedRequestId)
847868
{
848-
// Atomic guard: only one approval in-flight at a time.
849-
// If another approval is already running, skip this one entirely.
850869
if (Interlocked.CompareExchange(ref _autoApproveInFlight, e.RequestId, null) != null)
851870
{
852871
return;
853872
}
854873

874+
var approvalGeneration = Interlocked.Read(ref _generation);
875+
bool attemptedApprove = false;
876+
bool approved = false;
855877
try
856878
{
857879
var operatorClient = _activeLifecycle?.DataClient;
@@ -865,18 +887,10 @@ private async void OnNodePairingStatusChanged(object? sender, PairingStatusEvent
865887
_diagnostics.Record("node", $"Auto-approving node pairing (requestId={e.RequestId})");
866888
try
867889
{
868-
var approved = await operatorClient.NodePairApproveAsync(e.RequestId);
869-
if (approved)
870-
{
871-
_lastAutoApprovedRequestId = e.RequestId;
872-
_diagnostics.Record("node", "Node pairing auto-approved — reconnecting node");
873-
await Task.Delay(1000); // brief delay for gateway to process
874-
await StartNodeConnectionAsync();
875-
}
876-
else
877-
{
890+
attemptedApprove = true;
891+
approved = await operatorClient.NodePairApproveAsync(e.RequestId);
892+
if (!approved)
878893
_diagnostics.Record("node", "Node auto-approval failed");
879-
}
880894
}
881895
catch (Exception ex)
882896
{
@@ -888,8 +902,117 @@ private async void OnNodePairingStatusChanged(object? sender, PairingStatusEvent
888902
}
889903
finally
890904
{
905+
// Only dedupe after an actual approve attempt. If the operator
906+
// client was disconnected or lacked scope, the operator-side
907+
// NodePairListUpdated path must still be able to approve this
908+
// same requestId once the operator is ready.
909+
if (attemptedApprove && Interlocked.Read(ref _generation) == approvalGeneration)
910+
_lastAutoApprovedRequestId = e.RequestId;
911+
Interlocked.Exchange(ref _autoApproveInFlight, null);
912+
}
913+
914+
// Post-approve reconnect happens OUTSIDE the CAS guard so it
915+
// doesn't block unrelated approvals.
916+
if (approved)
917+
{
918+
_diagnostics.Record("node", "Node pairing auto-approved — reconnecting node");
919+
await Task.Delay(1000); // brief delay for gateway to process
920+
if (Interlocked.Read(ref _generation) == approvalGeneration)
921+
await StartNodeConnectionAsync();
922+
}
923+
}
924+
}
925+
926+
/// <summary>
927+
/// Operator-side auto-approve. When the gateway pushes
928+
/// <see cref="OpenClawGatewayClient.NodePairListUpdated"/> and there is a
929+
/// pending entry for our OWN node's deviceId, approve it. The node-side
930+
/// auto-approve at <see cref="OnNodePairingStatusChanged"/> handles the
931+
/// case where the node already knows it is pending; this method handles
932+
/// the case where the node is device-paired (its WindowsNodeClient sees
933+
/// itself as Paired) but its node-sub-pairing hasn't been approved yet —
934+
/// the only signal for that case is the operator-side broadcast.
935+
/// </summary>
936+
private async Task TryOperatorAutoApproveOwnNodePairAsync(PairingListInfo? info, long gen)
937+
{
938+
if (info?.Pending == null || info.Pending.Count == 0) return;
939+
940+
var ownNodeId = _nodeConnector?.NodeDeviceId;
941+
if (string.IsNullOrWhiteSpace(ownNodeId)) return;
942+
943+
var operatorClient = _activeLifecycle?.DataClient;
944+
if (operatorClient?.IsConnectedToGateway != true) return;
945+
if (!OperatorScopeHelper.CanApproveDevices(operatorClient.GrantedOperatorScopes)) return;
946+
947+
// Track whether ANY approve succeeded so we know to schedule one
948+
// reconnect at the end (rather than reconnecting per-entry, which
949+
// would race with itself).
950+
string? lastApprovedRequestId = null;
951+
952+
foreach (var req in info.Pending)
953+
{
954+
if (Interlocked.Read(ref _generation) != gen) return;
955+
if (string.IsNullOrWhiteSpace(req.RequestId)) continue;
956+
if (req.RequestId == _lastAutoApprovedRequestId) continue;
957+
if (!string.Equals(req.NodeId, ownNodeId, StringComparison.OrdinalIgnoreCase)) continue;
958+
959+
// CAS guard scoped to JUST the approve RPC. Release before the
960+
// post-approve reconnect so unrelated approvals are not starved
961+
// (e.g. another own-node pending with a different requestId in
962+
// the same or next snapshot).
963+
if (Interlocked.CompareExchange(ref _autoApproveInFlight, req.RequestId, null) != null)
964+
continue;
965+
966+
bool approved = false;
967+
try
968+
{
969+
_diagnostics.Record("node", $"Operator-side auto-approving own node pairing (requestId={req.RequestId})");
970+
try
971+
{
972+
approved = await operatorClient.NodePairApproveAsync(req.RequestId);
973+
if (!approved)
974+
_diagnostics.Record("node", "Operator-side node pair approval rejected by gateway");
975+
}
976+
catch (Exception ex)
977+
{
978+
_logger.Warn($"[ConnMgr] Operator-side node auto-approve failed: {ex.Message}");
979+
_diagnostics.Record("node", $"Operator-side auto-approve error: {ex.Message}");
980+
}
981+
}
982+
finally
983+
{
984+
// Always record the requestId — both on success (prevent
985+
// re-approving the same id after the gateway re-broadcasts)
986+
// and on failure (prevent a spin loop on a rejected id).
987+
// Re-check generation: if a reconnect happened during the
988+
// await above, DisposeActiveClient already cleared
989+
// _lastAutoApprovedRequestId for the new generation; we must
990+
// not overwrite that null with a stale id from the old gen.
991+
if (Interlocked.Read(ref _generation) == gen)
992+
_lastAutoApprovedRequestId = req.RequestId;
891993
Interlocked.Exchange(ref _autoApproveInFlight, null);
892994
}
995+
996+
if (approved)
997+
{
998+
lastApprovedRequestId = req.RequestId;
999+
// Continue scanning so a second own-pending in the same
1000+
// snapshot (e.g. stale requestId from prior session) also
1001+
// gets attempted — broken only by an explicit failure to
1002+
// approve, which we still try the next entry for.
1003+
}
1004+
// On failure/rejection, fall through to next own-pending entry
1005+
// rather than break — the gateway may not re-broadcast if the
1006+
// approve frame round-tripped and was rejected mid-flight.
1007+
}
1008+
1009+
// Single reconnect after the snapshot is fully processed.
1010+
if (lastApprovedRequestId is not null)
1011+
{
1012+
_diagnostics.Record("node", $"Operator-side approved {lastApprovedRequestId} — reconnecting node so caps propagate");
1013+
await Task.Delay(1000);
1014+
if (Interlocked.Read(ref _generation) == gen)
1015+
await StartNodeConnectionAsync();
8931016
}
8941017
}
8951018

src/OpenClaw.Shared/IOperatorGatewayClient.cs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,10 @@ public interface IOperatorGatewayClient
4141
string? OperatorDeviceId { get; }
4242
IReadOnlyList<string> GrantedOperatorScopes { get; }
4343
bool IsConnectedToGateway { get; }
44+
/// <summary>Canonical main session key resolved from hello-ok; <c>null</c> until handshake.</summary>
45+
string? MainSessionKey { get; }
46+
/// <summary>True once the hello-ok handshake has been processed.</summary>
47+
bool HasHandshakeSnapshot { get; }
4448

4549
// ─── Connection events (from WebSocketClientBase) ───
4650
event EventHandler<ConnectionStatus>? StatusChanged;

src/OpenClaw.Shared/OpenClaw.Shared.csproj

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
<ItemGroup>
1111
<InternalsVisibleTo Include="OpenClaw.Shared.Tests" />
12+
<InternalsVisibleTo Include="OpenClaw.Connection.Tests" />
1213
</ItemGroup>
1314

1415
<ItemGroup>

0 commit comments

Comments
 (0)