Skip to content

feat: add semantic Windows exec approval prompts - #1027

Draft
ruben2000de wants to merge 1 commit into
openclaw:mainfrom
ruben2000de:agent/semantic-approval-prompts
Draft

feat: add semantic Windows exec approval prompts#1027
ruben2000de wants to merge 1 commit into
openclaw:mainfrom
ruben2000de:agent/semantic-approval-prompts

Conversation

@ruben2000de

@ruben2000de ruben2000de commented Jul 19, 2026

Copy link
Copy Markdown

What changed

  • carry OpenClaw's canonical commandPreview through system.run.prepare, the legacy approval path, and the current V2 approval pipeline
  • preserve the existing Command / DisplayCommand contract as the exact authoritative command
  • render the exact command first and the preview as separate, explicitly agent-provided context afterward
  • sanitize explanatory context independently: preserve intentional line breaks, escape control/format/BiDi characters, and bound the rendered value
  • use the supplied application display name instead of a hard-coded agent name
  • keep Deny as the default-focused action and keep policy plus configured sandbox controls authoritative
  • add the new request-context label across every currently shipped WinUI locale

Why

The Windows approval prompt currently makes the raw command the operator's primary decision surface. OpenClaw already provides commandPreview as a human-readable explanation, but the Windows node did not preserve it through both approval pipelines.

The preview must remain presentation-only. It can help the operator understand purpose, impact, risk, and the recommended decision, but it must never replace the exact command used for policy, audit, or execution. This revision therefore keeps the exact command contract unchanged and adds the preview as a separate field and UI block.

Security and compatibility

  • policy evaluation and execution use only the canonical exact command/argv
  • legacy prompt handlers continue to receive the exact command in ExecApprovalPromptRequest.Command
  • V2 continues to derive DisplayCommand from canonical argv
  • the preview is never used for policy, executable resolution, allowlist persistence, logging, or execution
  • command text keeps the existing command sanitizer; explanatory context uses a separate bounded sanitizer that preserves multi-line structure while escaping spoofing characters
  • both legacy and V2 prompts place the exact command before untrusted preview text, preventing newline-heavy context from displacing the approval target

Validation

Passed locally on the rebased current head:

  • dotnet build src/OpenClaw.Shared/OpenClaw.Shared.csproj --no-restore -c Release — 0 warnings, 0 errors
  • 16 focused Shared regression tests — all passed
  • 16 Tray contract/localization validation tests — all passed
  • full Shared suite on Linux — 3,196 passed, 31 skipped; 37 Windows/platform or current-main environment failures
  • full Tray suite on Linux — 1,942 passed, 2 skipped; 11 Windows/GDI+/current-main source-environment failures

Real behavior proof

Not verified yet on the rebased head. The Linux container cannot execute MakePri.exe or XamlCompiler.exe, and the paired Windows validation node was offline during this revision.

Still required before marking ready for review:

  • ./build.ps1
  • Shared and Tray test projects on Windows
  • ./scripts/validate-mxc-e2e.ps1
  • a redacted current-head approval capture showing separate request context, the exact command, localized actions, and default Deny focus

@ruben2000de
ruben2000de force-pushed the agent/semantic-approval-prompts branch from 3b0c7fb to 48ece7e Compare July 19, 2026 18:17
@clawsweeper clawsweeper Bot added 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. labels Jul 19, 2026
@clawsweeper

clawsweeper Bot commented Jul 19, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed August 2, 2026, 1:58 PM ET / 17:58 UTC.

ClawSweeper review

What this changes

The PR carries agent-supplied command-preview text through Windows exec approvals and displays it as separately labeled, sanitized context after the exact command.

Merge readiness

Blocked until real behavior proof is added - 6 items remain

Keep this PR open, but it is not merge-ready. The feature is still absent from current main, while the branch also edits the legacy approval path that the merged V2 cutover retired; it needs a V2-only rebase and current-head Windows proof before review can pass.

Priority: P2
Reviewed head: a3c3fba8944a27da699c1f0e1ef17df135a84a68

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The patch has a focused intent and regression coverage, but current-main incompatibility and missing real Windows behavior proof make it unready for merge.
Proof confidence 🧂 unranked krab (1/6) Needs real behavior proof before merge: The PR body explicitly says the rebased head still lacks required Windows build, MXC E2E, MCP/gateway invocation, and a redacted approval-dialog capture. Add current-head proof and redact private paths, endpoints, and tokens; a PR-body update should trigger re-review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The PR body explicitly says the rebased head still lacks required Windows build, MXC E2E, MCP/gateway invocation, and a redacted approval-dialog capture. Add current-head proof and redact private paths, endpoints, and tokens; a PR-body update should trigger re-review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 4 items Current V2-only ownership: Current main builds ExecApprovalV2PromptRequest in the V2 coordinator and sends it to the V2 UI handler and dialog. The request type has no preview field, so the requested feature is not already implemented.
Legacy-path conflict: The proposed branch changes SystemCapability and ExecApprovalPromptService, but the merged V2 rewrite replaced the legacy approval architecture. The GitHub PR state is dirty against main, so the branch cannot be merged as submitted.
V2 migration provenance: The current approval-owner history identifies the merged V2 cutover as the latest architectural change, followed by the original V2 dialog implementation. This supports rebasing the feature onto V2 rather than restoring parallel legacy plumbing.
Findings 1 actionable finding [P1] Rebase the feature onto the V2-only approval path
Security None None.

How this fits together

Windows node system.run requests are structurally validated into canonical arguments, authorized by the V2 exec-approval coordinator, and shown in a local WinUI approval dialog before contained execution. This change adds non-authoritative agent context to that operator decision surface while preserving the exact command as the approval target.

flowchart LR
  A[Gateway system.run request] --> B[Windows node validation]
  B --> C[V2 approval coordinator]
  C --> D[WinUI approval dialog]
  D --> E[Operator allow or deny]
  E --> F[Contained command execution]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The PR body explicitly says the rebased head still lacks required Windows build, MXC E2E, MCP/gateway invocation, and a redacted approval-dialog capture. Add current-head proof and redact private paths, endpoints, and tokens; a PR-body update should trigger re-review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Rebase the feature onto the V2-only approval path (P1) - This changed legacy-path call site no longer exists on current main after the V2 approval cutover, while the PR is dirty against main. Rework the preview flow through the current V2 validator, coordinator, prompt request, UI handler, and dialog instead of attempting to retain the removed parallel path.
  • Resolve merge risk (P1) - Merging the branch as submitted is unsafe because it is dirty against current main and still includes changes to the retired legacy approval route.
  • Resolve merge risk (P1) - Agent-provided context appears during an authorization decision, so the rebased implementation must keep canonical command identity, policy evaluation, persistence, logging, and execution independent of preview text.
  • Resolve merge risk (P1) - The PR lacks required current-head Windows build, MXC E2E, MCP/gateway invocation, and visible approval-dialog proof.
  • Complete next step (P2) - The contributor must rebase a dirty legacy-era branch and provide real current-head Windows proof; this proof-dependent merge gate cannot be completed by an automated repair lane.

Findings

  • [P1] Rebase the feature onto the V2-only approval path — src/OpenClaw.Shared/Capabilities/SystemCapability.cs:301-320
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch surface 27 files affected, 570 additions, 33 deletions The branch crosses approval protocol plumbing, retired legacy code, current V2 presentation, five locale resources, and regression tests.

Merge-risk options

Maintainer options:

  1. Rebase onto the V2 approval owner (recommended)
    Retarget the preview data only through the current V2 validation, coordinator, prompt projection, and dialog, without restoring legacy services or authorization paths.
  2. Pause the feature
    Pause or close the branch if the contributor cannot replace the retired-path edits with a clean current-main V2 implementation and proof.

Technical review

Best possible solution:

Rebase the useful preview and sanitizer design onto the current V2-only validation, coordinator, UI-handler, and dialog seam; preserve canonical argv as the sole authorization and execution identity; then publish redacted Windows proof showing the exact command, separate request context, localized actions, and Deny-focused default behavior.

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

Not applicable. This PR adds a new approval-dialog context field rather than reporting a broken current-main behavior; source inspection confirms the V2 dialog currently has no preview field.

Is this the best way to solve the issue?

No. The preview design is reasonable, but this branch is not the best solution because its legacy-path edits conflict with the V2-only approval architecture now on main.

Full review comments:

  • [P1] Rebase the feature onto the V2-only approval path — src/OpenClaw.Shared/Capabilities/SystemCapability.cs:301-320
    This changed legacy-path call site no longer exists on current main after the V2 approval cutover, while the PR is dirty against main. Rework the preview flow through the current V2 validator, coordinator, prompt request, UI handler, and dialog instead of attempting to retain the removed parallel path.
    Confidence: 0.98

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P2: The proposed change affects a bounded but security-sensitive operator approval workflow.
  • merge-risk: 🚨 compatibility: The branch edits legacy approval surfaces removed by the current V2 cutover and is dirty against main.
  • merge-risk: 🚨 security-boundary: The change presents agent-supplied content during an operator authorization decision and must preserve separation from canonical command authorization.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body explicitly says the rebased head still lacks required Windows build, MXC E2E, MCP/gateway invocation, and a redacted approval-dialog capture. Add current-head proof and redact private paths, endpoints, and tokens; a PR-body update should trigger re-review, or a maintainer can request @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Current V2-only ownership: Current main builds ExecApprovalV2PromptRequest in the V2 coordinator and sends it to the V2 UI handler and dialog. The request type has no preview field, so the requested feature is not already implemented. (src/OpenClaw.Shared/ExecApprovals/ExecApprovalsCoordinator.cs:448, 8ac00c5f4977)
  • Legacy-path conflict: The proposed branch changes SystemCapability and ExecApprovalPromptService, but the merged V2 rewrite replaced the legacy approval architecture. The GitHub PR state is dirty against main, so the branch cannot be merged as submitted. (src/OpenClaw.Shared/Capabilities/SystemCapability.cs:301, a3c3fba8944a)
  • V2 migration provenance: The current approval-owner history identifies the merged V2 cutover as the latest architectural change, followed by the original V2 dialog implementation. This supports rebasing the feature onto V2 rather than restoring parallel legacy plumbing. (src/OpenClaw.Shared/ExecApprovals/ExecApprovalsCoordinator.cs:448, 4806eaac7c24)
  • Repository proof policy: Repository policy requires the Windows build, Shared and Tray tests, MXC E2E for exec-approval work, and current-head visible UI proof. The PR body explicitly says those Windows and MXC proof steps remain unverified. (AGENTS.md:33, 8ac00c5f4977)

Likely related people:

  • Barbara Kudiess: Authored the merged V2 cutover that now owns authorization and prompt routing. (role: V2 approval architecture contributor; confidence: high; commits: 4806eaac7c24; files: src/OpenClaw.Shared/ExecApprovals/ExecApprovalsCoordinator.cs, src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2PromptRequest.cs)
  • AlexAlves87: Authored the original V2 exec-approval dialog and prior V2 hardening commits. (role: V2 dialog and approval-flow contributor; confidence: high; commits: 76f4d52c3b5e, 54927b462839, 6c54e5e669c0; files: src/OpenClaw.Shared/ExecApprovals/ExecApprovalV2UiPromptHandler.cs, src/OpenClaw.Tray.WinUI/Dialogs/ExecApprovalDialog.cs)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Rebase the feature onto the current V2-only approval path and remove every retired legacy-path change.
  • Run the required Windows build, Shared and Tray suites, plus scripts/validate-mxc-e2e.ps1.
  • Post redacted current-head proof of the dialog and a real MCP or gateway approval invocation.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
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.

Workflow

  • 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.

@ruben2000de

Copy link
Copy Markdown
Author

Windows CI is needed for the final WinUI validation because the relevant MakePri/XAML tooling is Windows-only. The focused approval tests and the Shared Release build pass locally; when convenient, please approve the fork workflow runs for this draft PR.

@ruben2000de
ruben2000de force-pushed the agent/semantic-approval-prompts branch from 48ece7e to a3c3fba Compare July 25, 2026 14:18
@ruben2000de

Copy link
Copy Markdown
Author

Rebased onto current main and pushed a new head (a3c3fba). The two review defects are addressed: the exact command contract is preserved for legacy and V2 handlers, and all prompt copy uses the supplied display name. Agent-provided context is additive, sanitized, bounded, and rendered after the exact command. Focused Shared and localization/contract tests pass locally. The new Build and Test / CodeQL runs are currently action_required; please approve the fork workflows when convenient so I can finish the Windows validation and current-head behavior proof.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. and removed merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Jul 30, 2026
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. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant