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 July 25, 2026, 10:22 AM ET / 14:22 UTC.

ClawSweeper review

What this changes

The PR carries OpenClaw's existing command-preview text through legacy and V2 Windows execution approval paths, then displays it as sanitized localized context after the exact command.

Merge readiness

Blocked until real behavior proof is added - 4 items remain

Keep this draft PR open. The prior contract and display-name findings appear addressed in the current head, but the PR itself confirms that it still lacks required current-head Windows and MXC real-behavior proof for an approval-security UI change.

Priority: P2
Reviewed head: a3c3fba8944a27da699c1f0e1ef17df135a84a68

Review scores

Measure Result What it means
Overall readiness 🧂 unranked krab (1/6) The implementation and focused coverage look coherent after the prior findings were addressed, but missing real Windows and MXC proof is a merge gate for this approval-security change.
Proof confidence 🧂 unranked krab (1/6) Needs real behavior proof before merge: The PR body explicitly reports no current-head Windows or MXC real-behavior proof. Before merge, add redacted live Windows validation and an approval capture or runtime log; redact private endpoints, IP addresses, tokens, and other sensitive details. Updating the PR body should trigger a fresh review, or a maintainer can request one with @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 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The PR body explicitly reports no current-head Windows or MXC real-behavior proof. Before merge, add redacted live Windows validation and an approval capture or runtime log; redact private endpoints, IP addresses, tokens, and other sensitive details. Updating the PR body should trigger a fresh review, or a maintainer can request one with @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 5 items Current-head contract repair: The current PR diff keeps the legacy prompt request Command field as the exact command while adding CommandPreview as a separate optional presentation field, and the V2 path continues to derive DisplayCommand from canonical argv.
Prompt ordering and sanitization: The PR adds a distinct context sanitizer and renders the exact command before agent-supplied context in the WinUI approval dialog; focused tests cover propagation, ordering, and BiDi/control-character escaping.
Required proof remains absent: The PR body explicitly says real behavior has not been verified on the rebased head and lists the required Windows build, Shared and Tray tests, MXC E2E validation, and a redacted approval capture as still required.
Findings None None.
Security None None.

How this fits together

Windows node command execution receives system.run requests from the gateway and applies policy before presenting an approval prompt to the local operator. This change carries optional agent context into that prompt while the exact command remains the execution and policy decision surface.

flowchart LR
  A[Gateway system.run request] --> B[Windows node command preparation]
  B --> C[Approval policy and sandbox checks]
  C --> D[Exact command approval target]
  B --> E[Agent request context]
  E --> F[Context sanitizer]
  D --> G[Windows approval prompt]
  F --> G
  G --> H[Allow or deny execution]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The PR body explicitly reports no current-head Windows or MXC real-behavior proof. Before merge, add redacted live Windows validation and an approval capture or runtime log; redact private endpoints, IP addresses, tokens, and other sensitive details. Updating the PR body should trigger a fresh review, or a maintainer can request one with @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.
  • Resolve merge risk (P1) - The change affects the local operator security boundary: without a current-head Windows prompt capture and MXC E2E run, reviewers cannot confirm that exact-command prominence, default Deny focus, localization, and policy-authoritative behavior survive the native runtime path.
  • Resolve merge risk (P1) - The branch changes both legacy and V2 approval presentations across 27 files, so focused unit tests alone do not establish the real Windows and gateway-mediated behavior required by repository policy.
  • Complete next step (P2) - The remaining merge blocker is contributor-provided native Windows and MXC proof, which a repair lane cannot credibly fabricate from this read-only review.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Approval surface 27 files changed, 570 additions, 33 deletions The patch crosses command preparation, both approval pipelines, native UI, five locale resources, and focused tests, so native end-to-end proof matters.
Pipeline coverage 2 approval pipelines changed Both the legacy prompt and V2 WinUI handler now receive agent-supplied context and must agree on the authoritative command contract.

Merge-risk options

Maintainer options:

  1. Collect native approval proof before merge (recommended)
    Run the repository-required Windows build, Shared and Tray suites, and MXC E2E validation, then post a redacted current-head prompt capture showing the exact command, context, localized actions, and default Deny focus.
  2. Pause until a capable Windows host is available
    Keep the draft open rather than accepting unverified behavior in the operator approval boundary.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Run the required Windows and MXC validation on the current head, then add redacted runtime evidence that the prompt retains exact-command prominence and default Deny focus.

Technical review

Best possible solution:

Validate the rebased head on a capable Windows host, run the required MXC E2E path, and attach a redacted approval capture that visibly shows the exact command before localized agent context with Deny focused by default.

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

Not applicable. This PR adds presentation of an existing command-preview field rather than reporting a failing current-main behavior; the important verification is an after-fix native Windows approval flow.

Is this the best way to solve the issue?

Yes, conditionally. Separating bounded, sanitized agent context from the canonical command is a maintainable approach, provided the required Windows and MXC runtime proof confirms the prompt preserves the intended security boundary.

AGENTS.md: found and applied where relevant.

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

Labels

Label justifications:

  • P2: This is a bounded but security-relevant operator approval UX improvement whose required native proof is still outstanding.
  • merge-risk: 🚨 compatibility: The PR changes legacy and V2 approval request and display contracts that existing Windows node users depend on during command approvals.
  • merge-risk: 🚨 security-boundary: The PR renders agent-supplied text in a local permission prompt, so native proof is needed to verify prompt ordering, sanitization, and Deny-default behavior.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • 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 reports no current-head Windows or MXC real-behavior proof. Before merge, add redacted live Windows validation and an approval capture or runtime log; redact private endpoints, IP addresses, tokens, and other sensitive details. Updating the PR body should trigger a fresh review, or a maintainer can request one with @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-head contract repair: The current PR diff keeps the legacy prompt request Command field as the exact command while adding CommandPreview as a separate optional presentation field, and the V2 path continues to derive DisplayCommand from canonical argv. (src/OpenClaw.Shared/ExecApprovalPrompt.cs, a3c3fba8944a)
  • Prompt ordering and sanitization: The PR adds a distinct context sanitizer and renders the exact command before agent-supplied context in the WinUI approval dialog; focused tests cover propagation, ordering, and BiDi/control-character escaping. (src/OpenClaw.Tray.WinUI/Dialogs/ExecApprovalDialog.cs, a3c3fba8944a)
  • Required proof remains absent: The PR body explicitly says real behavior has not been verified on the rebased head and lists the required Windows build, Shared and Tray tests, MXC E2E validation, and a redacted approval capture as still required. (a3c3fba8944a)
  • Repository validation policy: The repository policy requires full build and Shared and Tray tests after every code change, plus scripts/validate-mxc-e2e.ps1 for MXC, system.run, approval, and Windows command-execution changes. That policy makes native validation material to this PR rather than optional polish. (AGENTS.md, f50af187865c)
  • Review continuity: The previous review identified preserving the exact legacy command and using the supplied display name. The rebased PR diff and author update address both, so they are not re-raised. (a3c3fba8944a)

Likely related people:

  • unidentified: The supplied review context identifies the affected current-main approval and WinUI paths but contains no reliable current-main author or blame metadata; routing should go to the people who recently own these paths. (role: current-main approval-path owner; confidence: low; files: src/OpenClaw.Shared/Capabilities/SystemCapability.cs, 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.

  • Run the required Windows build and Shared and Tray test projects on the rebased head.
  • Run scripts/validate-mxc-e2e.ps1 on an MXC-capable host without AllowSkip.
  • Post redacted current-head proof that shows exact-command ordering, request context, localized actions, and default Deny focus.

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.

History

Review history (1 earlier review cycle)
  • reviewed 2026-07-19T18:20:25.137Z sha 48ece7e :: needs real behavior proof before merge. :: [P1] Keep Command as the exact command for legacy presenters | [P2] Use the supplied display name in the prompt copy

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

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