Chat: render in-bubble Allow/Deny approval banner + harden plumbing - #567
Merged
Conversation
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>
|
Codex review: needs changes before merge. Reviewed June 1, 2026, 3:46 PM ET / 19:46 UTC. Summary Reproducibility: yes. from source inspection: the PR clears PendingPermission and stamps Allowed/Denied immediately after sending the slash command, while terminal approval handling only checks PendingPermission. I did not run the Windows app because this review is read-only. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Mantis proof suggestion Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Land the inline approval UI after matching gateway terminal events drive or correct the final approval badge, with regression tests for allow, deny, timeout/expiry, rejection, and mismatched approval ids. Do we have a high-confidence way to reproduce the issue? Yes, from source inspection: the PR clears PendingPermission and stamps Allowed/Denied immediately after sending the slash command, while terminal approval handling only checks PendingPermission. I did not run the Windows app because this review is read-only. Is this the best way to solve the issue? No. The maintainable path is to keep a pending/awaiting state until a matching gateway terminal event confirms the outcome, or to allow that event to correct an optimistic state. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 1d58d59673c9. Label changesLabel changes:
Label justifications:
Evidence reviewedSecurity concerns:
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
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
Bot
added
rating: 🧂 unranked krab
Contributor
Author
|
Screenshot of initial combo boxes population on first start: Visible circles in light theme: Fixed content-block seam: Gateway approval request UI in the Dashboard for reference: Native chat approval UI: |
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
clawsweeper
Bot
added
rating: 🌊 off-meta tidepool
…d 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>
clawsweeper
Bot
added
proof: 📸 screenshot
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>
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 openclaw#567 plus two rounds of adversarial code review (Opus 4.6 + Sonnet 4.6). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Author
|
A few more screenshots and logs for the approval process: Gateway Node Configuration allowing approval prompts: Side-by-side approval UI for the HTML Dashboard and Windows Companion: Approval after expiration is ineffective: Effective approval: And the logs for that approval: [2026-05-28 14:15:41.430] [INFO] [EVENT] Received event: exec.approval.requested |
Contributor
Author
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
clawsweeper
Bot
added
proof: sufficient
clawsweeper
Bot
added
status: ⏳ waiting on author
RBrid
marked this pull request as ready for review
May 28, 2026 21:48
clawsweeper
Bot
added
rating: 🦪 silver shellfish
clawsweeper
Bot
added
rating: 🧂 unranked krab
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Renders the gateway-side
exec.approval.requestedevent as an inline Allow/Deny banner inside the assistant's chat bubble, instead of relying solely on the dashboard modal. Also hardens the chat data-provider plumbing, fixes a contrast bug in the setup wizard's pending-step circle, and fixes a combo-box population bug that surfaced after a gateway reset.What
OpenClawChatDataProvidersurfaces pendingexec.approval.requestedpermissions as inline content blocks rendered directly in the assistant message bubble. Allow/Deny actions route back through the existing approval lifecycle via/approve <slug> allow-onceor/deny <slug>slash commands, and the banner clears onphase=resolved.Path.Combine/System.IO.File. NewContentBlockSeamRepairTestscover positive and negative fixtures.IChatGatewayBridgesubscribe-first-then-reconcile pattern around the new approval path.ProgressPagepending-step badge now uses the theme-awareControlStrongStrokeColorDefaultBrushinstead of a hard-coded translucent white that was invisible on light backgrounds.Why
The dashboard
ContentDialogapproval modal is easy to miss when the chat window has focus. Rendering the prompt inline keeps the user in flow and matches the gateway's intent of streaming the approval request alongside the agent's response. The combo-box and contrast fixes were small drive-bys on the same branch — the pending-step ring was effectively invisible in light mode, and the combo box was reproducibly empty after a gateway reset.Validation
./build.ps1✅OpenClaw.Shared.Tests: 2023 passed / 29 skipped ✅OpenClaw.Tray.Tests: 890 passed (2 pre-existingStartupSetupStateTestsfailures unrelated to this branch)Review
Went through 5 rounds of adversarial dual-model review (Opus + Codex) before commit. All consensus HIGH findings addressed.