Skip to content

Add inline exec approval flow#751

Merged
shanselman merged 3 commits into
openclaw:mainfrom
RBrid:user/rbrid/InlineApproval
Jun 25, 2026
Merged

Add inline exec approval flow#751
shanselman merged 3 commits into
openclaw:mainfrom
RBrid:user/rbrid/InlineApproval

Conversation

@RBrid

@RBrid RBrid commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR routes local system.run exec-approval prompts into the native chat timeline when an interactive native chat surface is available, while preserving the native approval dialog as the fallback when chat is not visible/available.

Key behavior:

  • Shows inline command approval cards in chat with Allow once, Always allow, and Deny once actions.
  • Allow once approves only the current request.
  • Always allow approves the current request and persists an exact allow rule to exec-policy.json when the command can be safely represented exactly.
  • Deny once denies only the current request and does not persist a deny rule.
  • Inline approval cards expire after the gateway approval window, collapse to an expired state, and no longer leave enabled approval buttons behind.
  • Pending inline approvals also create a persistent app notification breadcrumb with an Open chat action that opens/selects the relevant chat session.
  • The notification uses localized display labels such as "Main chat" instead of raw session keys like agent:main:main; raw keys are kept only for routing.
  • If no native chat surface is active, or inline routing fails, the original native approval dialog remains the fallback path.

Policy and compatibility fixes

  • Thread session keys through node.invoke and system.run approval paths so local prompts can route to the correct chat session.
  • Keep legacy exec-policy compatibility by accepting canonical prompt, legacy ask, and numeric enum action values.
  • Preserve stable ChatPermissionDecision enum values.
  • Fix the Permissions page to read/write the same exec-policy.json path that NodeService enforces, including isolated OPENCLAW_TRAY_DATA_DIR runs.
  • Remove the load-time policy rewrite that could project rules through the reduced UI model.
  • Preserve policy fields (shells, description, enabled) on explicit save.
  • Omit default enabled: true on save while preserving enabled: false.
  • Keep explicit deny rules as hard denies; only prompt rules/defaults show approval UI.

Review-driven fixes addressed

  • ClawSweeper: stopped load-time policy rewrites and preserved full exec-policy rule fields.
  • Hanselman/Codex: fixed tray chat active-state cleanup, notification chat-action dedupe, terminal approval phase mapping, pending notification lifecycle, stale deny notification clearing after manual approval, and user-facing session labels.
  • Localized new/changed user-facing strings across supported locales and updated localization validation coverage.

Proof screenshots

Type of (cut-off) UI that moved inline into chat history:

image

Existing inline approval style for comparison:

image

Resulting inline approval UI:

image

After clicking Deny once:

image

After clicking Allow once:

image

After clicking Always allow:

image

Then subsequent matching requests do not require re-approval:

image

Permissions rule action dropdown fix:

image

Testing and review

Latest validation:

  • Build: ./build.ps1 passed.
  • Shared tests: dotnet test ./tests/OpenClaw.Shared.Tests/OpenClaw.Shared.Tests.csproj --no-restore passed (2374 passed, 29 skipped).
  • Tray tests: dotnet test ./tests/OpenClaw.Tray.Tests/OpenClaw.Tray.Tests.csproj --no-restore passed (1149 passed).
  • Localization validation: LocalizationValidationTests passed; new resource keys are present across supported locales with placeholder parity.

Manual testing performed with an isolated data directory:

  • Verified Permissions edits persist to the same isolated exec-policy.json enforced by the node.
  • Verified default prompt policy triggers inline approval for tasklist.
  • Verified Deny once, Allow once, and Always allow flows.
  • Verified Always allow persists an exact allow rule and survives app restart.
  • Verified repeated matching request runs without re-approval after restart.
  • Verified inline approval timeout collapses the approval card and disables action buttons.
  • Verified pending approval notification opens/selects the relevant chat session.

Review notes:

  • Bundled autoreview was run with the available Copilot engine because the default codex CLI is not installed in this environment; the run reported no accepted/actionable findings.
  • Multiple Hanselman dual-model reviews were run during iteration. Accepted/actionable findings were addressed and followed by the validation above.

@clawsweeper

clawsweeper Bot commented Jun 12, 2026

Copy link
Copy Markdown

Codex review: needs maintainer review before merge. Reviewed June 24, 2026, 10:19 PM ET / 02:19 UTC.

Summary
The PR routes local system.run approval prompts into native chat with allow-once, always-allow, deny, expiry, notification breadcrumbs, session-key routing, exec-policy compatibility fixes, localization, and tests.

Reproducibility: not applicable. this is a feature PR rather than a current-main bug report. Source inspection shows current main still uses the native prompt-only path, and the PR screenshots show the proposed inline behavior.

Review metrics: 2 noteworthy metrics.

  • Diff Surface: 37 files, +1781/-242. The branch spans shared node invocation, exec policy, tray chat UI, notifications, permissions, localization, and tests, so maintainer review should treat it as cross-surface work.
  • Proof Inspected: 3 screenshots inspected from 8 listed. The inspected artifacts directly show the pending inline card, always-allow result with command output, and Permissions dropdown behavior.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🦞 diamond lobster ✨ media proof bonus
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.

Mantis proof suggestion
A short desktop proof would still help maintainers verify the security-sensitive approval flow end to end before merge. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis visual task: verify inline command approval allow-once, always-allow, deny, expiry, notification open-chat, and native dialog fallback behavior.

Risk before merge

  • [P2] The PR moves local command-execution approval actions into chat, so maintainers still need to accept the security-boundary UX even though the native dialog fallback remains.
  • [P1] Exec-policy parsing and saving are compatibility-sensitive for existing users with legacy ask values, numeric actions, disabled rules, shell-scoped rules, or descriptions.
  • [P1] At inspection, the GitHub Build and Test rollup still had test and e2e jobs in progress, so merge should wait for the required checks to finish green.

Maintainer options:

  1. Land After Boundary Signoff (recommended)
    A maintainer can accept the compatibility and security-boundary risk once required checks are green and the provided proof is considered enough for the approval flow.
  2. Require Focused Upgrade Proof
    Before merge, ask for an explicit upgrade note or added proof covering legacy exec-policy.json files with numeric actions, shell-scoped rules, descriptions, and disabled rules.
  3. Split If Policy Scope Is Too Broad
    If maintainers do not want policy persistence fixes coupled to inline approval UI, pause this PR and split the compatibility work into a narrower branch.

Next step before merge

  • [P2] No narrow automation repair remains; the next action is maintainer review of the approval boundary, exec-policy compatibility, and final check results.

Security
Cleared: No concrete security or supply-chain defect was found in the diff, but the change remains security-boundary work requiring maintainer signoff.

Review details

Best possible solution:

Merge only after maintainer signoff accepts the inline approval boundary, exact allow-rule persistence, legacy exec-policy round-trip behavior, and final green validation.

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

Not applicable: this is a feature PR rather than a current-main bug report. Source inspection shows current main still uses the native prompt-only path, and the PR screenshots show the proposed inline behavior.

Is this the best way to solve the issue?

Yes with maintainer approval: session-key threading, native-chat routing only when available, timeout expiry, and native fallback are a maintainable path. The remaining question is maintainer acceptance of the command-approval UX and policy compatibility contract.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a substantial but bounded improvement to local exec approval and tray settings, not an active outage or emergency.
  • merge-risk: 🚨 compatibility: The PR changes exec-policy parsing and saving, so existing user policy files must continue to round-trip without downgraded rules.
  • merge-risk: 🚨 security-boundary: The PR changes where users approve local command execution and how always-allow decisions are persisted.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (screenshot): The PR body screenshots, including inspected downloaded artifacts, show the after-fix inline approval UI, always-allow result with command output, and Permissions dropdown behavior.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body screenshots, including inspected downloaded artifacts, show the after-fix inline approval UI, always-allow result with command output, and Permissions dropdown behavior.
  • proof: 📸 screenshot: Contributor real behavior proof includes screenshot evidence. The PR body screenshots, including inspected downloaded artifacts, show the after-fix inline approval UI, always-allow result with command output, and Permissions dropdown behavior.
Evidence reviewed

What I checked:

  • Repository policy read: AGENTS.md was read fully; its node/tray UX guidance and required validation policy affected the review, while the read-only ClawSweeper contract prevented running the build and tests locally. (AGENTS.md:1, 9f4d23804122)
  • Current main still uses native prompt path: Current main's ExecApprovalPromptService directly starts the native prompt thread and has no inline chat routing or chat-provider hook, so the PR's central behavior is not already implemented. (src/OpenClaw.Tray.WinUI/Services/ExecApprovalPromptService.cs:35, 9f4d23804122)
  • PR head adds inline routing with fallback: The PR head attempts chat-based local approval when a session key and native chat surface are available, and falls back to the native prompt if inline routing is unavailable or fails. (src/OpenClaw.Tray.WinUI/Services/ExecApprovalPromptService.cs:54, ae301c096d67)
  • Inline approval implementation: The PR head creates local inline approval timeline entries, expires pending approvals, and resolves allow-once, always-allow, and deny decisions back to the prompt task. (src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs:977, ae301c096d67)
  • Exec-policy compatibility path: The PR head loads legacy action spellings and numeric values, preserves shells, description, and disabled state, and saves the same exec-policy.json path that NodeService enforces. (src/OpenClaw.Tray.WinUI/Pages/PermissionsPage.xaml.cs:600, ae301c096d67)
  • Real behavior proof inspected: Downloaded screenshots show the inline approval card with Allow once, Always allow, and Deny once, an always-allow result with command output, and the Permissions action dropdown including Ask. (ae301c096d67)

Likely related people:

  • RBrid: Current-main history includes prior app notification, exec-policy, native chat, and approval delivery work in the same surfaces touched by this PR, beyond authorship of the branch. (role: recent area contributor; confidence: high; commits: 06ed71f3c01f, 5505a85da7df; files: src/OpenClaw.Tray.WinUI/Services/ExecApprovalPromptService.cs, src/OpenClaw.Shared/ExecApprovalPolicy.cs, src/OpenClaw.Tray.WinUI/Chat/OpenClawChatDataProvider.cs)
  • ranjeshj: Blame and file history attribute the existing native prompt path and recent Permissions page foundation to this author in current main. (role: current-main introducer and adjacent tray UX contributor; confidence: medium; commits: a9bc121e0846, 429be9ba9368; files: src/OpenClaw.Tray.WinUI/Services/ExecApprovalPromptService.cs, src/OpenClaw.Tray.WinUI/Pages/PermissionsPage.xaml.cs)
  • Barbara Kudiess: Current main's latest commit touched NodeService while porting adjacent MXC and pairing approval work, which intersects the node capability wiring affected by this PR. (role: recent NodeService area contributor; confidence: medium; commits: 9f4d23804122; files: src/OpenClaw.Tray.WinUI/Services/NodeService.cs)
  • shanselman: The PR discussion includes a security-boundary review note from this reviewer, and the latest PR head commit fixes a tray chat flow issue in the branch. (role: discussion reviewer and recent PR committer; confidence: medium; commits: ae301c096d67; files: src/OpenClaw.Tray.WinUI/Windows/ChatWindow.xaml.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 the rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. label Jun 12, 2026
@RBrid

RBrid commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 12, 2026

Copy link
Copy Markdown

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@RBrid
RBrid marked this pull request as ready for review June 12, 2026 22:21
@shanselman

Copy link
Copy Markdown
Collaborator

Maintainer note: this is a substantial exec-approval UI flow and it is currently conflicting. Given the size and the security boundary involved, I don't think this is safe to resolve opportunistically during triage. Please rebase on current main (especially after the exec approvals persistence work that just landed), then add a short proof section showing the allow/deny/remember flows and the tests that cover them.

@clawsweeper clawsweeper Bot added proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. 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. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. proof: sufficient Contributor real behavior proof is sufficient. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. 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. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. 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. labels Jun 22, 2026
RBrid and others added 3 commits June 24, 2026 21:55
Route local exec approval prompts into the active chat timeline when session context is available, including allow-once, always-allow, deny, resolved-state stamping, and local prompt cancellation handling. Preserve native prompt fallback for requests without chat context.

Thread session keys through node.invoke request/event paths and system.run approvals while preserving trusted envelope metadata over command args. Keep legacy policy compatibility by accepting ask/prompt/numeric action values and preserving stable ChatPermissionDecision enum values.

Update the permissions UI to use canonical prompt actions, upsert exact duplicate command patterns, safely coalesce duplicate loaded rules without changing first-match semantics, and refresh localized approval copy for allow-once versus always-allow behavior.

Add regression coverage for inline approval decisions, session-key propagation, duplicate policy rule handling, legacy policy deserialization, enum stability, and action-button fallback behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep Permissions editing the same exec policy enforced by the node, preserve policy fields, add pending approval notifications, and make inline approvals expire with the gateway timeout.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove a duplicate Show() call introduced while hardening the inline exec approval tray chat flow.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@shanselman
shanselman force-pushed the user/rbrid/InlineApproval branch from 5537a3f to ae301c0 Compare June 25, 2026 02:14
@shanselman
shanselman merged commit 4e7982b into openclaw:main Jun 25, 2026
14 checks passed
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. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. proof: sufficient Contributor real behavior proof is sufficient. 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.

2 participants