Skip to content

fix(approvals): summarize long approval prompts#80141

Open
HemantSudarshan wants to merge 1 commit into
openclaw:mainfrom
HemantSudarshan:hemant/58687-approval-prompt-summary
Open

fix(approvals): summarize long approval prompts#80141
HemantSudarshan wants to merge 1 commit into
openclaw:mainfrom
HemantSudarshan:hemant/58687-approval-prompt-summary

Conversation

@HemantSudarshan

@HemantSudarshan HemantSudarshan commented May 10, 2026

Copy link
Copy Markdown
Contributor

Problem summary

Fixes #58687. Long exec approval prompts can render enough command text that the approval controls become hard to reach or review, especially for inline scripts. The Control UI also rendered an Allow Always button even when the gateway omitted allow-always from the request's allowed decisions, making that action look ineffective.

Root cause

Exec approval request payloads were already sanitized/redacted before display, but presentation surfaces still rendered the whole sanitized command up to the existing display cap. The Control UI parser also ignored request.allowedDecisions, so it could offer a decision that the gateway would reject for ask: "always" or other restricted approval modes.

Architectural reasoning

  • Adds a browser-safe command prompt summarizer in src/infra and uses it only at presentation boundaries: fallback forwarded approval text, shared approval reply payloads, native approval view models, and the Control UI modal.
  • Leaves the pending approval record, sanitized command payload, systemRunPlan, bindings, and resolution path unchanged, so command identity and allow-always matching are not loosened.
  • Preserves short command rendering exactly and only summarizes when the prompt exceeds five logical lines or 1,200 display chars.
  • Teaches the Control UI approval parser to carry allowedDecisions for exec and plugin approval requests, then renders only those actions in the modal.

Real behavior proof

  • Behavior or issue addressed: long exec approval command text is summarized before prompt rendering, and Control UI approval parsing preserves the gateway-provided allowed decisions so unavailable Allow Always choices can be omitted.
  • Real environment tested: local OpenClaw checkout on Windows, Node v22.15.0, pnpm 10.33.2, branch hemant/58687-approval-prompt-summary at aaa32699bfff52d4147d2850f456eaa2c88f3dbe.
  • Exact steps or command run after this patch:
    1. Imported the actual runtime helper from src/infra/exec-approval-command-summary.ts with pnpm exec tsx.
    2. Built an eight-line approval command using the same escaped line-break marker shape that sanitized approval command text can contain.
    3. Parsed a synthetic gateway exec.approval.requested payload through the actual Control UI parser with allowedDecisions: ["allow-once", "deny"].
    4. Printed the resulting prompt summary and parsed decisions.
  • Evidence after fix:
$ pnpm exec tsx -e "import { summarizeExecApprovalCommandForPrompt } from './src/infra/exec-approval-command-summary.ts'; import { parseExecApprovalRequested } from './ui/src/ui/controllers/exec-approval.ts'; const command = Array.from({ length: 8 }, (_, i) => 'echo line ' + (i + 1) + '\\u{A}').join(''); const summary = summarizeExecApprovalCommandForPrompt(command); const parsed = parseExecApprovalRequested({ id: 'proof-1', request: { command: 'echo ok', allowedDecisions: ['allow-once', 'deny'] }, createdAtMs: 1, expiresAtMs: 2 }); console.log(JSON.stringify({ summaryTruncated: summary.truncated, hiddenLineCount: summary.hiddenLineCount, includesLine8: summary.text.includes('line 8'), summaryPreview: summary.text.split('\n').slice(0, 6), parsedAllowedDecisions: parsed?.request.allowedDecisions }, null, 2));"
{
  "summaryTruncated": true,
  "hiddenLineCount": 4,
  "includesLine8": false,
  "summaryPreview": [
    "echo line 1\\u{A}",
    "echo line 2\\u{A}",
    "echo line 3\\u{A}",
    "echo line 4\\u{A}",
    "echo line 5\\u{A}",
    "...[truncated: showing first 5 of 9 lines; 52 chars hidden]"
  ],
  "parsedAllowedDecisions": [
    "allow-once",
    "deny"
  ]
}
  • Observed result after fix: the prompt summary is truncated, hides the tail (includesLine8: false), reports the hidden content, and the parser returns only allow-once and deny, which is the decision set the modal will render.
  • What was not tested: I did not attach a screenshot because the browser modal path is covered by ui/src/ui/views/exec-approval.browser.test.ts; I did not run a live gateway approval through an external chat account because the changed code is presentation-only and the native adapter tests cover Matrix, Slack, and Telegram rendering.

Testing performed

  • pnpm test src/infra/exec-approval-command-summary.test.ts src/infra/exec-approval-reply.test.ts src/infra/exec-approval-forwarder.test.ts src/infra/approval-view-model.test.ts ui/src/ui/controllers/exec-approval.test.ts ui/src/ui/views/exec-approval.test.ts ui/src/ui/views/exec-approval.browser.test.ts src/infra/approval-handler-runtime.test.ts extensions/matrix/src/approval-handler.runtime.test.ts extensions/slack/src/approval-handler.runtime.test.ts extensions/telegram/src/approval-handler.runtime.test.ts
  • pnpm exec oxfmt --check --threads=1 CHANGELOG.md docs/tools/exec-approvals-advanced.md src/infra/exec-approval-command-summary.ts src/infra/exec-approval-command-summary.test.ts src/infra/approval-view-model.ts src/infra/approval-view-model.test.ts src/infra/exec-approval-forwarder.ts src/infra/exec-approval-forwarder.test.ts src/infra/exec-approval-reply.ts src/infra/exec-approval-reply.test.ts ui/src/ui/controllers/exec-approval.ts ui/src/ui/controllers/exec-approval.test.ts ui/src/ui/views/exec-approval.ts ui/src/ui/views/exec-approval.test.ts
  • pnpm lint:core
  • pnpm docs:check-mdx
  • pnpm lint:docs
  • pnpm tsgo:core via temporary P:\ mapping because direct execution from C:\Projects\Personal Project\openclaw still trips the path-with-spaces launcher issue
  • pnpm tsgo:core:test via temporary P:\ mapping
  • pnpm tsgo:test:ui via temporary P:\ mapping
  • pnpm check:changed
  • git diff --check upstream/main...HEAD

pnpm test:changed fanned out to the broad unit-fast shard and failed on unrelated Windows/local-environment assumptions: POSIX path expectation mismatches, table glyph fallback expectations, SQLite temp cleanup EBUSY, missing A2UI scaffold returning 404, and Testbox local-key fixture assumptions. The focused approval, UI, and native adapter tests above passed.

Risk analysis

Low-to-medium. The changed behavior is intentionally display-only except for honoring already-present allowedDecisions in the Control UI. Approval matching, pending record storage, node/gateway resolution, command binding, and execution semantics are unchanged. The main risk is that native approval clients now receive summarized view.commandText for long commands, covered by native handler tests across Matrix, Slack, and Telegram.

Screenshots/logs

No screenshot was needed; the browser approval modal regression test covers the rendered modal path. The real behavior proof and test command summaries above are the relevant logs.

Backward compatibility

Existing approval request payload shape is compatible. Clients that do not send allowedDecisions still see the historical default actions. Short command prompts render unchanged; long prompts are summarized only in human-facing approval surfaces while the full sanitized command remains bound to the approval record.

@openclaw-barnacle openclaw-barnacle Bot added docs Improvements or additions to documentation app: web-ui App: web-ui size: M proof: supplied External PR includes structured after-fix real behavior proof. labels May 10, 2026
@clawsweeper

clawsweeper Bot commented May 10, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 3, 2026, 11:52 PM ET / 03:52 UTC.

Summary
The PR adds long exec approval command summarization across shared/native, fallback, reply, and Control UI prompt surfaces, carries Control UI allowed decisions, and updates related tests, docs, and changelog text.

PR surface: Source +162, Tests +153, Docs +2. Total +317 across 14 files.

Reproducibility: yes. from source inspection and linked screenshots: current main renders full sanitized command text into Control UI, native, and fallback approval prompt surfaces below the hard cap. I did not run a live prompt in this read-only review.

Review metrics: 1 noteworthy metric.

  • Approval display threshold: 1 added: 5 logical lines or 1,200 chars. This changes what operators can see before approving a command, so maintainers need to review the security and usability tradeoff before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #58687
Summary: This PR is a candidate fix for the linked long approval prompt issue; related approval PRs overlap only part of the allow-always or prompt-readability surface.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦪 silver shellfish
Patch quality: 🧂 unranked krab
Result: blocked until stronger real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Add a full sanitized-command reveal, inspect, or copy path before approval.
  • Refresh the branch against current main while preserving allowed-decision fallback, unavailable Allow Always warning, and no-deny-on-cancel behavior.
  • [P1] Add redacted browser modal or native-channel prompt proof that shows the after-fix prompt and decisions.

Proof guidance:

  • [P1] Needs stronger real behavior proof before merge: The PR body has terminal helper/parser output from synthetic payloads, but it still needs redacted browser, native-channel, or Telegram prompt proof showing the after-fix prompt and decisions; after updating the PR body, ClawSweeper should re-review automatically or a maintainer can request @clawsweeper re-review.

Mantis proof suggestion
A Telegram Desktop recording would show the visible native approval prompt text, allowed buttons, and whether the full sanitized command is inspectable after the PR is updated. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis telegram desktop proof: verify a long exec approval prompt stays reviewable, preserves allowed decisions, and exposes the full sanitized command before approval.

Risk before merge

  • [P1] Summary-only approval prompts can hide executable command tail content while the approval still applies to the full pending command.
  • [P2] The branch is conflicting and overlaps current-main ask-aware allowed-decision fallback, unavailable Allow Always warning, and no-deny-on-cancel behavior.
  • [P1] The supplied proof is terminal helper/parser output from synthetic payloads, not a real browser modal, native channel, or Telegram prompt showing the after-fix user flow.
  • [P1] The PR edits release-owned CHANGELOG.md for a normal PR; release-note context should stay in the PR body or squash metadata.

Maintainer options:

  1. Require Inspectable Approval Summaries (recommended)
    Add a full sanitized-command reveal, inspect, or copy path before summarized prompts can approve the underlying command, then prove that path in a real prompt surface.
  2. Refresh Against Current Approval Semantics
    Rebase or replace the branch while preserving ask-aware fallback, unavailable Allow Always warning, and no-deny-on-cancel behavior from current main.
  3. Pause For Maintainer Approval Boundary Decision
    If maintainers want summary-only prompts without a full reveal path, they should explicitly accept that approval-boundary tradeoff before this proceeds.

Next step before merge

  • [P1] Human review is needed because the remaining blockers are an approval-boundary UX/security choice, current-main conflict preservation, and contributor real-prompt proof that automation cannot supply.

Security
Needs attention: The diff changes an approval boundary and can let users approve the full command after seeing only a summary of it.

Review findings

  • [P1] Keep the full command inspectable before approval — src/infra/approval-view-model.ts:78
  • [P3] Remove the release-owned changelog edit — CHANGELOG.md:14
Review details

Best possible solution:

Land a current-main-based approval presentation fix that keeps long prompts reviewable, exposes or copies the full sanitized command before approval, preserves current decision semantics, and leaves release notes outside CHANGELOG.md.

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

Yes from source inspection and linked screenshots: current main renders full sanitized command text into Control UI, native, and fallback approval prompt surfaces below the hard cap. I did not run a live prompt in this read-only review.

Is this the best way to solve the issue?

No. Summarizing long prompts is a plausible mitigation, but the best approval-boundary fix must keep the full sanitized command inspectable before approval and preserve current-main approval decision behavior.

Full review comments:

  • [P1] Keep the full command inspectable before approval — src/infra/approval-view-model.ts:78
    This shared approval view replaces the displayed command with only the summary for long commands. Native and fallback approval surfaces can then let an operator approve the full pending command after seeing only the prefix, so the prompt needs a full sanitized-command reveal, inspect, or copy path before approval.
    Confidence: 0.91
  • [P3] Remove the release-owned changelog edit — CHANGELOG.md:14
    Normal PRs should keep release-note context in the PR body or squash message because CHANGELOG.md is release-owned. This entry should be removed from the branch and carried as PR-body release context instead.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.89

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P1: The PR targets a real approval workflow bug while changing a security-sensitive command approval surface users rely on before execution.
  • merge-risk: 🚨 compatibility: The conflicting branch overlaps newer current-main fallback, warning, and cancel semantics that must survive any conflict resolution.
  • merge-risk: 🚨 security-boundary: Summarized approval prompts can hide executable command content while approval still applies to the full command.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: The PR body has terminal helper/parser output from synthetic payloads, but it still needs redacted browser, native-channel, or Telegram prompt proof showing the after-fix prompt and decisions; after updating the PR body, ClawSweeper should re-review automatically or a maintainer can request @clawsweeper re-review.
  • mantis: telegram-visible-proof: Mantis should capture Telegram visible proof. The shared native approval view feeds visible Telegram approval prompt text and buttons, so this can be demonstrated in a short Telegram Desktop recording.
Evidence reviewed

PR surface:

Source +162, Tests +153, Docs +2. Total +317 across 14 files.

View PR surface stats
Area Files Added Removed Net
Source 6 186 24 +162
Tests 6 155 2 +153
Docs 2 2 0 +2
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 14 343 26 +317

Security concerns:

  • [high] Approval prompt hides executable command tail — src/infra/approval-view-model.ts:78
    The summarizer feeds commandSummary.text into shared commandText consumed by native approval clients, while approval resolution still applies to the full pending command; an operator may authorize command content they did not see.
    Confidence: 0.91

What I checked:

  • Repository policy read: Read the full root AGENTS.md plus scoped UI and docs guidance; approval security boundaries, current-main review, real behavior proof, and release-owned changelog policy affected this review. (AGENTS.md:1, 9d68f877ac3e)
  • Linked user bug remains relevant: The linked issue reports long security prompts that become hard to approve and ineffective Allow All behavior on OpenClaw 2026.3.31, with screenshots and supporting comments.
  • PR head summarizes shared native command text: At PR head, buildExecViewBase stores commandSummary.text as commandText, so shared/native approval consumers receive the shortened text for long commands. (src/infra/approval-view-model.ts:78, aaa32699bfff)
  • Current main shows full sanitized command in shared views: Current main resolves the sanitized command display and passes commandText through without soft summarization in the shared approval view model. (src/infra/approval-view-model.ts:65, 9d68f877ac3e)
  • Telegram native approvals consume shared commandText: Telegram pending approval delivery builds the pending reply payload from params.view.commandText, so the shared summary changes visible Telegram approval text. (extensions/telegram/src/approval-handler.runtime.ts:83, 9d68f877ac3e)
  • Current main has newer allowed-decision semantics: Current main already parses allowedDecisions, falls back from ask=always to allow-once/deny, warns when Allow Always is unavailable, and avoids Escape denial when deny is unavailable. (ui/src/ui/views/exec-approval.ts:142, 9d68f877ac3e)

Likely related people:

  • jesse-merhi: Authored current-main work hiding unavailable durable approval actions and earlier command-span/highlighting work in the Control UI approval surface. (role: recent approval UI semantics contributor; confidence: high; commits: 42f0822bfac9, 297a16453661; files: ui/src/ui/views/exec-approval.ts, ui/src/ui/controllers/exec-approval.ts)
  • steipete: Recent and repeated contributor across exec approval command analysis, display, forwarding, and infra ownership paths. (role: feature-history area contributor; confidence: medium; commits: 3f7e6eebc2f5, 493076671101; files: src/infra/exec-approval-forwarder.ts, src/infra/exec-approval-command-display.ts, src/infra/approval-view-model.ts)
  • pgondhi987: Authored the current-main hard display-limit rejection for exec approval registration, which overlaps the security side of this PR. (role: adjacent approval security hardening contributor; confidence: medium; commits: 731af9c96bba; files: src/gateway/server-methods/exec-approval.ts)
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 mantis: telegram-visible-proof Mantis should capture Telegram visible proof. label May 14, 2026
@openclaw-barnacle

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jun 1, 2026
@clawsweeper clawsweeper Bot added rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. and removed mantis: telegram-visible-proof Mantis should capture Telegram visible proof. labels Jun 1, 2026
@openclaw-barnacle openclaw-barnacle Bot removed the stale Marked as stale due to inactivity label Jun 2, 2026
@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. mantis: telegram-visible-proof Mantis should capture Telegram visible proof. P1 High-priority user-facing bug, regression, or broken workflow. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. and removed rating: 🌊 off-meta tidepool PR readiness rating does not apply to this item. labels Jun 14, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 21, 2026
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 23, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 29, 2026
@openclaw-barnacle

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jul 19, 2026
@clawsweeper

clawsweeper Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper status: review started.

I am starting a fresh review of this pull request: fix(approvals): summarize long approval prompts This is item 1/1 in the current shard. Shard 18/20.

This placeholder means the worker is alive and reading the current context. I will edit this same comment with the actual review when the claws are done clicking.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

app: web-ui App: web-ui docs Improvements or additions to documentation mantis: telegram-visible-proof Mantis should capture Telegram visible proof. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. merge-risk: 🚨 security-boundary 🚨 May affect sandboxing, authorization, credentials, or sensitive data. P1 High-priority user-facing bug, regression, or broken workflow. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: M stale Marked as stale due to inactivity 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.

[Bug]: Security prompt is too long to approve, and clicking "Allow All" on the security prompt is ineffective

1 participant