Skip to content

Commit 7d9152f

Browse files
RBridCopilot
andauthored
Chat: render in-bubble Allow/Deny approval banner + harden plumbing (#567)
* Chat: render in-bubble Allow/Deny approval banner + harden plumbing Render gateway-emitted exec.approval.requested events as an interactive banner in the chat composer (header + plain-English subtitle + monospace command + caption + Allow/Deny buttons), matching the dashboard modal experience but inline. Allow/Deny posts a /approve <slug> allow-once or /deny <slug> slash command back to the gateway; the banner clears on phase=resolved. Feature - ChatTimelineReducer: preserve PendingPermission across follow-on tool item events (previously wiped ~1ms after set). - OpenClawChatDataProvider: full approval lifecycle (map requested, track id-asymmetry between approvalSlug and approvalId, clear on positive request-id match only). - OpenClawComposer + Resources.resw (en, fr, nl, zh-CN, zh-TW): banner copy + Allow/Deny buttons. Plumbing hardening - Slash-command echo handling: pre-register /approve|/deny in the local-echo queue when sent, suppress on our own echo, surface remote-issued slashes as dim audit-trail status entries (instead of rendering them as user bubbles). chat.history rebuild applies the same audit rendering. FetchRemoteUserMessage filters slash commands. - Slash matcher uses a strict regex (^/(?:approve\s+[A-Za-z0-9_-]{4,64}(?:\s+allow-once)?|deny\s+...)$) to avoid accidentally suppressing user text that starts with /approve. - ClearPendingPermissionAndPublish takes an expectedRequestId so a fresh approval that arrives between the user's tap and the post-send clear is not stomped. - Terminal-clear path inverted to clear only on positive id match; empty or no-match -> preserve banner + log. - Approval LRU is instance-scoped, cleared on disconnect; alt-id map shares LRU eviction with the dedupe set. - On RespondToPermissionAsync send failure: remove the pre-registered slash entry from the local-echo queue so it doesn't block subsequent echo consumption for 30s. - IChatGatewayBridge: subscribe-first-then-reconcile ctor pattern with an idempotent Disconnected seed so an in-flight Connecting edge is preserved. Content-block seam repair - Tighten the s_seamSentencePunct regex to require a multi-char trailing Pascal word AND a non-EOS terminator ((?<=[a-z0-9][.!?:])(?=[A-Z][a-z]+[\s,;:!?])). Eliminates a class of false positives where the regex was shredding identifiers like Path.Combine and System.IO.File. Add ContentBlockSeamRepairTests with both positive and negative fixtures. - char.ToUpper -> char.ToUpperInvariant on agent labels in OpenClawChatRoot to keep label casing stable across locales. Tests - ContentBlockSeamRepairTests (new): positive seam-repair cases from real Sonnet/Opus captures + negative EOS fixtures. - ChatTimelineReducerTests, OpenClawChatDataProviderTests, ToolMetaCacheTests: extend coverage of new lifecycle paths. ProgressPage.xaml.cs: minor unrelated tidy that was on the branch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix nullability warnings in OpenClawChatRoot after warning-clean-build enforcement Master enabled TreatWarningsAsErrors (commit 6bbc74b "eng: enforce warning-clean builds"). After merging master, the existing `effectiveThread.Id` usage in the composer construction and its lambda callbacks tripped CS8604 because `ChatThread.Id` is typed `string?` even though the surrounding `effectiveThread is not null` guard implies it's safe to access. Add `!` null-forgiving operators to `effectiveThread.Id` in the composer props (ChannelId, synthetic ChannelGroup Sessions entry) and in the OnSend/OnStop/OnPermissionResponse/OnModelChanged/ OnThinkingLevelChanged/OnPermissionsChanged callbacks so the build stays clean under the new warning-as-error policy. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Salt ChatTrace hash with per-process random seed The [ChatTrace] log line included a stable FNV-1a hash of every chat message so two near-duplicate frames could be told apart while hunting the duplicate-bubble bug. ClawSweeper flagged that a stable content- derived hash is itself a content fingerprint: anyone with the log can rainbow-table common short messages back to their hash and confirm what was sent. Keep the diagnostic by seeding FNV-1a with a per-process random uint from RandomNumberGenerator. Within one tray run, identical text still collides (so duplicate-frame detection still works), but the hash is not reproducible from a guessed plaintext and rotates on every restart. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Move chat permission approval UI into the timeline Approval requests previously rendered as a floating bubble pinned above the composer, which made them disappear from history once resolved and made it impossible to scroll back to confirm what was approved/denied. This change folds the approval flow into ChatTimelineReducer so each request becomes a real ChatTimelineItem alongside user/assistant/tool entries. Reducer changes (OpenClaw.Chat): - New ChatTimelineItemKind.PermissionRequest and ChatPermissionDecision enum (Pending/Allowed/Denied/Expired). - ApplyPermissionRequest expires any prior Pending entries, appends the new request, and sets PendingPermission. Empty/whitespace RequestId events are dropped to avoid orphaning a Pending entry whose buttons are permanently disabled. - ResolvePermission stamps the matching entry without downgrading an already-decided one (gateway backstop events can't clobber a local Allow/Deny click). Mismatched RequestId is a no-op for both entries and PendingPermission. - ClearPermission delegates to ResolvePermission(Expired). WinUI changes: - RenderPermissionEntry in OpenClawChatTimeline renders the live bubble (Allow/Deny buttons) and decided badges (Allowed / Denied / Expired) inline. Decided detail is truncated at 120 chars with surrogate-safe boundary handling so emoji/CJK supplementary chars don't render as U+FFFD. - AutomationProperties.Name on Allow/Deny buttons and the decided badge so screen readers hear 'Allow shell.exec' / 'Allowed run script.sh' instead of bare glyphs. Omits the suffix when no IntentSummary is set to avoid awkward 'Allow Approval needed' phrasing. - OpenClawComposer no longer renders the floating bubble. - OpenClawChatRoot's isEmptyConversation guard documents that the pendingPermissionOverride check is now preview-only (live data always has entries.Count > 0 when PendingPermission != null). Localization: - Added Chat_Permission_DecisionAllowed/Denied/Expired keys in all five locales (en-us, fr-fr, nl-nl, zh-cn, zh-tw). Tests: +ResolvePermission_MismatchedRequestId_NoOp, +ApplyPermissionRequest_EmptyRequestId_DroppedToAvoidOrphanedEntry, and extended ResolvePermission_Denied_StampsEntry with a PendingPermission null-assertion. Validated: ./build.ps1, Shared 2023/29 skipped, Tray 897/0. Addresses ClawSweeper feedback on PR #567 plus two rounds of adversarial code review (Opus 4.6 + Sonnet 4.6). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Nudge ClawSweeper Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent ec11986 commit 7d9152f

17 files changed

Lines changed: 1428 additions & 70 deletions

src/OpenClaw.Chat/ChatModels.cs

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,32 @@ public enum ChatTimelineItemKind
2323
ToolCall,
2424
Reasoning,
2525
Status,
26-
Raw
26+
Raw,
27+
PermissionRequest
28+
}
29+
30+
/// <summary>
31+
/// Outcome of an exec-approval prompt, attached to a
32+
/// <see cref="ChatTimelineItemKind.PermissionRequest"/> timeline entry.
33+
/// </summary>
34+
/// <remarks>
35+
/// <para><see cref="Pending"/> is the initial state — Allow/Deny buttons
36+
/// render and the matching <see cref="ChatTimelineState.PendingPermission"/>
37+
/// slot is non-null.</para>
38+
/// <para><see cref="Allowed"/> / <see cref="Denied"/> are set locally as
39+
/// soon as the user clicks a button, so the inline bubble collapses to a
40+
/// "decided" badge without waiting for the gateway round-trip.</para>
41+
/// <para><see cref="Expired"/> is the backstop set when the gateway emits
42+
/// a terminal approval phase (resolved / cancelled / timed-out) before the
43+
/// user picked an option — e.g. another client decided, or the gateway
44+
/// timed the prompt out. Visually distinguishes it from a user choice.</para>
45+
/// </remarks>
46+
public enum ChatPermissionDecision
47+
{
48+
Pending,
49+
Allowed,
50+
Denied,
51+
Expired
2752
}
2853

2954
public enum ChatToolCallStatus
@@ -76,7 +101,9 @@ public record ChatTimelineItem(
76101
string? IntentSummary = null,
77102
JsonObject? ToolArgs = null,
78103
ChatTone? Tone = null,
79-
string? ToolCallId = null);
104+
string? ToolCallId = null,
105+
string? PermissionRequestId = null,
106+
ChatPermissionDecision PermissionDecision = ChatPermissionDecision.Pending);
80107

81108
public record ChatPermissionRequest(string RequestId, string PermissionKind, string ToolName, string Detail);
82109

src/OpenClaw.Chat/ChatTimelineReducer.cs

Lines changed: 105 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,7 @@ public static ChatTimelineState Apply(ChatTimelineState state, ChatEvent evt)
2424
ChatRestoredEvent e => PushEntry(state, ChatTimelineItemKind.Status, e.Text, ChatTone.Info),
2525
ChatContextChangedEvent => state,
2626
ChatModelChangedEvent e => PushEntry(state, ChatTimelineItemKind.Status, $"Model -> {e.Model}", ChatTone.Success),
27-
ChatPermissionRequestEvent e => state with
28-
{
29-
PendingPermission = new ChatPermissionRequest(e.RequestId, e.PermissionKind, e.ToolName, e.Detail)
30-
},
27+
ChatPermissionRequestEvent e => ApplyPermissionRequest(state, e),
3128
ChatRawEvent e => e.Text is { Length: > 0 } t ? PushEntry(state, ChatTimelineItemKind.Raw, t) : state,
3229
_ => state
3330
};
@@ -59,7 +56,102 @@ public static ChatTimelineState AddSystem(ChatTimelineState state, string text,
5956
=> PushEntry(state, ChatTimelineItemKind.Status, text, tone);
6057

6158
public static ChatTimelineState ClearPermission(ChatTimelineState state)
62-
=> state with { PendingPermission = null };
59+
=> ResolvePermission(state, requestId: state.PendingPermission?.RequestId, decision: ChatPermissionDecision.Expired);
60+
61+
/// <summary>
62+
/// Marks the timeline entry for <paramref name="requestId"/> with a
63+
/// terminal <paramref name="decision"/> and (if it is the live one)
64+
/// clears <see cref="ChatTimelineState.PendingPermission"/>.
65+
/// </summary>
66+
/// <remarks>
67+
/// <para>This is the source of truth for "the inline approval bubble
68+
/// is now decided". UI callers route Allow/Deny clicks here with
69+
/// <see cref="ChatPermissionDecision.Allowed"/> / <see cref="ChatPermissionDecision.Denied"/>
70+
/// so the bubble collapses to its decided badge immediately, without
71+
/// waiting for the gateway round-trip.</para>
72+
/// <para>Gateway-side terminal events (the legacy ClearPermission
73+
/// path) call this with <see cref="ChatPermissionDecision.Expired"/>
74+
/// as a backstop in case the user never clicked — visually
75+
/// distinguishes "decided by user" from "decided elsewhere or timed
76+
/// out".</para>
77+
/// <para>If <paramref name="requestId"/> is null or no matching entry
78+
/// exists, the entry list is left untouched and only
79+
/// <see cref="ChatTimelineState.PendingPermission"/> is cleared (mirrors
80+
/// the prior ClearPermission contract).</para>
81+
/// <para>Entries whose <see cref="ChatTimelineItem.PermissionDecision"/>
82+
/// is already non-Pending are not overwritten — once the user has made
83+
/// a choice locally, a later gateway "Expired" event won't downgrade it.</para>
84+
/// </remarks>
85+
public static ChatTimelineState ResolvePermission(ChatTimelineState state, string? requestId, ChatPermissionDecision decision)
86+
{
87+
var entries = state.Entries;
88+
if (!string.IsNullOrEmpty(requestId))
89+
{
90+
for (int i = entries.Count - 1; i >= 0; i--)
91+
{
92+
var entry = entries[i];
93+
if (entry.Kind != ChatTimelineItemKind.PermissionRequest) continue;
94+
if (!string.Equals(entry.PermissionRequestId, requestId, StringComparison.Ordinal)) continue;
95+
if (entry.PermissionDecision != ChatPermissionDecision.Pending) break;
96+
entries = entries.SetItem(i, entry with { PermissionDecision = decision });
97+
break;
98+
}
99+
}
100+
101+
var clearedPending = state.PendingPermission is null
102+
|| (requestId is null)
103+
|| string.Equals(state.PendingPermission.RequestId, requestId, StringComparison.Ordinal)
104+
? null
105+
: state.PendingPermission;
106+
107+
return state with { Entries = entries, PendingPermission = clearedPending };
108+
}
109+
110+
static ChatTimelineState ApplyPermissionRequest(ChatTimelineState state, ChatPermissionRequestEvent e)
111+
{
112+
// A second exec-approval can arrive before the first is resolved.
113+
// Mark any still-Pending prior approval entry as Expired so the
114+
// timeline doesn't show two live Allow/Deny prompts at once — the
115+
// gateway has implicitly superseded the earlier one by issuing a
116+
// new approval. This mirrors the prior single-slot PendingPermission
117+
// behavior, which silently replaced the older request.
118+
var entries = state.Entries;
119+
for (int i = entries.Count - 1; i >= 0; i--)
120+
{
121+
var existing = entries[i];
122+
if (existing.Kind != ChatTimelineItemKind.PermissionRequest) continue;
123+
if (existing.PermissionDecision != ChatPermissionDecision.Pending) continue;
124+
entries = entries.SetItem(i, existing with { PermissionDecision = ChatPermissionDecision.Expired });
125+
}
126+
127+
var detail = e.Detail;
128+
// Defensive: an empty/whitespace RequestId in the gateway event
129+
// would otherwise be committed to state. ResolvePermission's
130+
// entry-scan guard skips on IsNullOrEmpty, so PendingPermission
131+
// would be cleared by ClearPermission while the Pending entry
132+
// stays stuck with disabled buttons. Drop such malformed events.
133+
if (string.IsNullOrWhiteSpace(e.RequestId))
134+
{
135+
return state;
136+
}
137+
138+
var id = $"e{state.NextId}";
139+
var entry = new ChatTimelineItem(
140+
id,
141+
ChatTimelineItemKind.PermissionRequest,
142+
detail,
143+
ToolName: e.ToolName,
144+
IntentSummary: e.PermissionKind,
145+
PermissionRequestId: e.RequestId,
146+
PermissionDecision: ChatPermissionDecision.Pending);
147+
148+
return state with
149+
{
150+
Entries = entries.Add(entry),
151+
NextId = state.NextId + 1,
152+
PendingPermission = new ChatPermissionRequest(e.RequestId, e.PermissionKind, e.ToolName, detail)
153+
};
154+
}
63155

64156
static ChatTimelineState ApplyUserMessage(ChatTimelineState state, ChatUserMessageEvent e)
65157
{
@@ -123,7 +215,11 @@ static ChatTimelineState ApplyToolOutput(ChatTimelineState state, ChatToolOutput
123215
// Don't remove from ActiveToolCalls here: multiple output events can arrive
124216
// for the same tool (command_output + item end). Mapping is cleared at turn end.
125217
ActiveToolCallId = (entryId == state.ActiveToolCallId) ? null : state.ActiveToolCallId,
126-
PendingPermission = null
218+
// NOTE: PendingPermission intentionally preserved. Exec-approval
219+
// events interleave with tool item events (chip start → approval
220+
// → tool output), so wiping the banner on tool output would race
221+
// it off-screen. Callers clear via ClearPermission on user click
222+
// or on phase=resolved.
127223
};
128224
}
129225

@@ -147,7 +243,7 @@ static ChatTimelineState ApplyToolError(ChatTimelineState state, ChatToolErrorEv
147243
Entries = entries,
148244
ActiveToolCallId = (entryId == state.ActiveToolCallId) ? null : state.ActiveToolCallId,
149245
ActiveToolCalls = e.ToolCallId is { } k ? state.ActiveToolCalls.Remove(k) : state.ActiveToolCalls,
150-
PendingPermission = null
246+
// PendingPermission preserved — see ApplyToolOutput note.
151247
};
152248
}
153249

@@ -312,7 +408,8 @@ static ChatTimelineState ApplyTurnEnd(ChatTimelineState state)
312408
ActiveReasoningId = null,
313409
ActiveToolCallId = null,
314410
ActiveToolCalls = System.Collections.Immutable.ImmutableDictionary<string, string>.Empty,
315-
PendingPermission = null
411+
// PendingPermission preserved — exec approvals may outlive their
412+
// originating turn (gateway emits phase=resolved to clear).
316413
};
317414
}
318415

src/OpenClaw.SetupEngine.UI/Pages/ProgressPage.xaml.cs

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -332,14 +332,28 @@ public void SetStatus(StepStatus status)
332332

333333
private static Border CreateEmptyBadge()
334334
{
335-
return new Border
335+
// Use a theme-aware stroke brush so the pending-step ring is visible
336+
// in both light and dark mode. The previous hard-coded translucent
337+
// white was invisible against light backgrounds.
338+
var border = new Border
336339
{
337340
Width = 22,
338341
Height = 22,
339342
CornerRadius = new CornerRadius(11),
340343
BorderThickness = new Thickness(1),
341-
BorderBrush = new SolidColorBrush(Color.FromArgb(80, 255, 255, 255)),
342344
};
345+
346+
if (Application.Current.Resources.TryGetValue("ControlStrongStrokeColorDefaultBrush", out var brush)
347+
&& brush is Brush themed)
348+
{
349+
border.BorderBrush = themed;
350+
}
351+
else
352+
{
353+
border.BorderBrush = new SolidColorBrush(Color.FromArgb(140, 128, 128, 128));
354+
}
355+
356+
return border;
343357
}
344358

345359
private static Border CreateIconBadge(string glyph, Color background, Color foreground)

src/OpenClaw.Tray.WinUI/Chat/IChatGatewayBridge.cs

Lines changed: 65 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using OpenClaw.Shared;
2+
using OpenClawTray.Services;
23

34
namespace OpenClawTray.Chat;
45

@@ -18,6 +19,21 @@ public interface IChatGatewayBridge : IDisposable
1819
SessionInfo[] GetSessionList();
1920
ModelsListInfo? GetCurrentModelsList();
2021

22+
/// <summary>
23+
/// If the underlying gateway client was already Connected by the time
24+
/// the bridge was constructed (so the bridge missed the
25+
/// <see cref="StatusChanged"/> → Connected edge), proactively re-request
26+
/// the models list and sessions snapshot so the chat composer's
27+
/// dropdowns populate without waiting for the user to send a message.
28+
///
29+
/// Callers should invoke this AFTER subscribing to
30+
/// <see cref="ModelsListUpdated"/> and <see cref="SessionsUpdated"/> so
31+
/// they actually receive the resulting frames — firing the request
32+
/// before subscription leaves the response handler unset and the
33+
/// dropdowns stale until the next gateway-driven update.
34+
/// </summary>
35+
void StartProactiveBootstrap();
36+
2137
Task SendChatMessageAsync(string message, string? sessionKey, string? sessionId, IReadOnlyList<ChatAttachment>? attachments = null);
2238
Task PatchSessionModelAsync(string sessionKey, string model);
2339
Task PatchSessionThinkingLevelAsync(string sessionKey, string thinkingLevel);
@@ -42,7 +58,16 @@ public sealed class GatewayClientChatBridge : IChatGatewayBridge
4258
private readonly EventHandler<ChatMessageInfo> _chatMessageReceivedHandler;
4359
private readonly EventHandler<AgentEventInfo> _agentEventReceivedHandler;
4460
private readonly EventHandler<ModelsListInfo> _modelsListUpdatedHandler;
45-
private ConnectionStatus _currentStatus = ConnectionStatus.Disconnected;
61+
// _currentStatus is written from the gateway client's StatusChanged
62+
// callback (arbitrary thread) and read from CurrentStatus on the UI
63+
// thread. ``volatile`` gives us a memory barrier so the reader can't
64+
// observe a torn or stale value after the writer fires. Atomicity for
65+
// ConnectionStatus (4-byte enum) is guaranteed by the CLR.
66+
//
67+
// Seeded from the client's current state *before* the StatusChanged
68+
// handler is subscribed (see ctor) so a real StatusChanged edge that
69+
// fires during construction can't be stomped back by a stale seed.
70+
private volatile ConnectionStatus _currentStatus = ConnectionStatus.Disconnected;
4671
private ModelsListInfo? _currentModels;
4772
private bool _disposed;
4873

@@ -58,6 +83,7 @@ public GatewayClientChatBridge(OpenClawGatewayClient client)
5883
// the Hub's SessionsPage first.
5984
if (e == ConnectionStatus.Connected)
6085
{
86+
Logger.Info("[ChatBridge] StatusChanged→Connected: requesting models.list");
6187
_ = _client.RequestModelsListAsync();
6288
}
6389
};
@@ -70,11 +96,48 @@ public GatewayClientChatBridge(OpenClawGatewayClient client)
7096
ModelsListUpdated?.Invoke(s, e);
7197
};
7298

99+
// Subscribe StatusChanged BEFORE reading the seed so any
100+
// ``StatusChanged → X`` edge that fires during construction is
101+
// captured by our handler. We then read the live property and
102+
// reconcile ``_currentStatus`` so callers that hit
103+
// ``CurrentStatus`` immediately (before any further edge) see
104+
// truth rather than the default ``Disconnected``.
105+
//
106+
// The seed only writes if ``_currentStatus`` is still
107+
// ``Disconnected`` (its default). If a handler edge fired in
108+
// the subscribe→read window — including intermediate states
109+
// like ``Connecting`` — the handler's write is preserved
110+
// rather than collapsed by the 2-state read of
111+
// ``IsConnectedToGateway``. ``volatile`` covers atomic reads.
73112
_client.StatusChanged += _statusChangedHandler;
74113
_client.SessionsUpdated += _sessionsUpdatedHandler;
75114
_client.ChatMessageReceived += _chatMessageReceivedHandler;
76115
_client.AgentEventReceived += _agentEventReceivedHandler;
77116
_client.ModelsListUpdated += _modelsListUpdatedHandler;
117+
118+
if (_currentStatus == ConnectionStatus.Disconnected)
119+
{
120+
_currentStatus = _client.IsConnectedToGateway
121+
? ConnectionStatus.Connected
122+
: ConnectionStatus.Disconnected;
123+
}
124+
125+
Logger.Info($"[ChatBridge] ctor: IsConnectedToGateway={_client.IsConnectedToGateway}");
126+
// The actual proactive models.list/sessions.list request is
127+
// deferred to StartProactiveBootstrap() — kicking it off here
128+
// would race the provider's subscription to ModelsListUpdated:
129+
// the response can arrive before the provider has wired its
130+
// handler, leaving the composer dropdowns empty until the next
131+
// gateway-driven update.
132+
}
133+
134+
public void StartProactiveBootstrap()
135+
{
136+
if (_disposed) return;
137+
if (!_client.IsConnectedToGateway) return;
138+
Logger.Info("[ChatBridge] proactive: requesting models.list and sessions.list");
139+
try { _ = _client.RequestModelsListAsync(); } catch (Exception ex) { Logger.Warn($"[ChatBridge] proactive models.list failed: {ex.Message}"); }
140+
try { _ = _client.RequestSessionsAsync(); } catch (Exception ex) { Logger.Warn($"[ChatBridge] proactive sessions.list failed: {ex.Message}"); }
78141
}
79142

80143
public bool IsConnected => _client.IsConnectedToGateway;
@@ -122,3 +185,4 @@ public void Dispose()
122185
ModelsListUpdated = null;
123186
}
124187
}
188+

0 commit comments

Comments
 (0)