Skip to content

fix(approvals): summarize long approval prompts#80024

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

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

Conversation

@HemantSudarshan

@HemantSudarshan HemantSudarshan commented May 9, 2026 •
edited
Loading

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 codex/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 labels May 9, 2026
@openclaw-barnacle openclaw-barnacle Bot added the triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. label May 10, 2026
@HemantSudarshan
HemantSudarshan force-pushed the codex/58687-approval-prompt-summary branch from ddee59e to aaa3269 Compare May 10, 2026 00:00
@openclaw-barnacle openclaw-barnacle Bot added proof: supplied External PR includes structured after-fix real behavior proof. and removed triage: needs-real-behavior-proof Candidate: external PR needs after-fix proof from a real setup. labels May 10, 2026
@clawsweeper

clawsweeper Bot commented May 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs changes before merge.

Summary
The PR adds a shared exec-approval command summarizer, applies it to Control UI and approval delivery surfaces, carries gateway allowedDecisions through Control UI parsing/rendering, and updates tests, docs, and changelog.

Reproducibility: yes. Source inspection of current main shows the Control UI renders the full command and ignores allowed decisions, and the linked user report describes a concrete long-script approval prompt path; I did not run a live gateway approval in this read-only review.

Real behavior proof
Sufficient (terminal): The PR body includes terminal output from a real Windows checkout running the actual new helper/parser and showing the after-fix summary plus parsed allowed decisions.

Next step before merge
A narrow automated repair can make the summarization path idempotent or single-pass and add a regression test for Matrix/Telegram native payload text.

Security
Needs attention: No dependency or supply-chain change was found, but the approval prompt's hidden-command metadata can be distorted in native delivery paths.

Review findings

  • [P2] Avoid re-summarizing native approval payloads — src/infra/exec-approval-reply.ts:306-309
Review details

Best possible solution:

Keep the presentation-boundary summarizer and allowed-decision propagation, but make summarization single-pass/idempotent across native/shared payload builders while preserving the full sanitized pending record for matching.

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

Yes. Source inspection of current main shows the Control UI renders the full command and ignores allowed decisions, and the linked user report describes a concrete long-script approval prompt path; I did not run a live gateway approval in this read-only review.

Is this the best way to solve the issue?

No, not as-is. The chosen direction is maintainable, but the patch should avoid summarizing already-summarized native view text before it is safe to merge.

Full review comments:

  • [P2] Avoid re-summarizing native approval payloads — src/infra/exec-approval-reply.ts:306-309
    buildPendingApprovalView now stores a summarized view.commandText, and Matrix/Telegram pass that value into buildExecApprovalPendingReplyPayload. Summarizing params.command again here treats the first truncation marker as another hidden line, so native approval text can report showing first 5 of 6 lines instead of the original command's hidden-line/char counts. Make this helper idempotent or pass unsummarized display text into the shared payload builder.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.86

Security concerns:

  • [low] Preserve accurate truncation metadata — src/infra/exec-approval-reply.ts:306
    Approval prompts are security-sensitive review surfaces; double summarization can understate how much command content is hidden in Matrix/Telegram native prompts, which weakens the operator's review signal even though command binding remains unchanged.
    Confidence: 0.82

Acceptance criteria:

  • pnpm test src/infra/exec-approval-command-summary.test.ts src/infra/exec-approval-reply.test.ts src/infra/approval-view-model.test.ts extensions/matrix/src/approval-handler.runtime.test.ts extensions/telegram/src/approval-handler.runtime.test.ts
  • pnpm test 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
  • pnpm exec oxfmt --check --threads=1 src/infra/exec-approval-command-summary.ts src/infra/exec-approval-reply.ts src/infra/approval-view-model.ts extensions/matrix/src/approval-handler.runtime.test.ts extensions/telegram/src/approval-handler.runtime.test.ts ui/src/ui/controllers/exec-approval.ts ui/src/ui/views/exec-approval.ts

What I checked:

  • Current Control UI renders full command text and all decisions: On current main, renderCommandWithSpans renders request.command directly when there are no spans, and the modal always renders Allow once, Allow Always, and Deny buttons. (ui/src/ui/views/exec-approval.ts:55, d5fe89abb536)
  • Current Control UI parser drops allowedDecisions: On current main, parseExecApprovalRequested and parsePluginApprovalRequested build request payloads without copying request.allowedDecisions, even though gateway responses include those decisions. (ui/src/ui/controllers/exec-approval.ts:96, d5fe89abb536)
  • Gateway already supplies allowed decisions: The exec approval get handler includes allowedDecisions: resolveExecApprovalRequestAllowedDecisions(...), so the UI omission is a presentation/parser issue rather than a missing gateway contract. (src/gateway/server-methods/exec-approval.ts:119, d5fe89abb536)
  • PR diff summarizes both view model and shared reply payload: The patch summarizes view.commandText in buildExecViewBase and also summarizes params.command in buildExecApprovalPendingReplyPayload, which creates a double-summary path for native clients that pass view.commandText into the shared payload builder. (src/infra/exec-approval-reply.ts:306, aaa32699bfff)
  • Native Matrix and Telegram pass view.commandText to the shared payload builder: Matrix and Telegram native approval runtimes build pending exec payloads with command: params.view.commandText, so after this patch they would pass already-summarized text into a second summarization step. (extensions/matrix/src/approval-handler.runtime.ts:318, d5fe89abb536)
  • Linked user report confirms the real UX problem: The linked issue and comments describe long security prompts, including a 500-line Python-script approval prompt that becomes unusable on mobile, plus confusion around Allow All behavior.

Likely related people:

  • @steipete: GitHub path history shows repeated recent approval infra and Telegram/runtime refactors, including refactor(approvals): unify structured path display on the shared reply payload area. (role: approval infra maintainer; confidence: high; commits: 32dd1ffc5a7f, bd0e10a2f68f, f0000ab72d01; files: src/infra/exec-approval-reply.ts, src/gateway/server-methods/exec-approval.ts, extensions/telegram/src/approval-handler.runtime.ts)
  • @jesse-merhi: Authored the recent merged Web approval command-risk highlighting work that changed gateway validation, Control UI parsing/rendering, and exec approval tests in this same surface. (role: recent Web approval contributor; confidence: high; commits: 297a16453661; files: ui/src/ui/views/exec-approval.ts, ui/src/ui/controllers/exec-approval.ts, src/gateway/server-methods/exec-approval.ts)
  • @BunsDev: Authored the recent modal migration for exec approval prompts, including the dialog/focus behavior around the exact Control UI surface changed here. (role: Control UI modal maintainer; confidence: medium; commits: e5a5ea10722e; files: ui/src/ui/views/exec-approval.ts, ui/src/ui/components/modal-dialog.ts)
  • @pashpashpash: Recent Telegram diagnostics and native sensitive-command approval work touched the native approval delivery path that would receive summarized view text. (role: native approval adjacent owner; confidence: medium; commits: 6ce1058296cc; files: extensions/telegram/src/approval-handler.runtime.ts, extensions/telegram/src/exec-approval-forwarding.ts)

Remaining risk / open question:

Codex review notes: model gpt-5.5, reasoning high; reviewed against d5fe89abb536.

@clawsweeper clawsweeper Bot added the proof: sufficient ClawSweeper judged the real behavior proof convincing. label May 10, 2026
@HemantSudarshan
HemantSudarshan deleted the codex/58687-approval-prompt-summary branch May 10, 2026 06:59
@HemantSudarshan HemantSudarshan mentioned this pull request May 10, 2026
Open
@HemantSudarshan

HemantSudarshan commented May 10, 2026

Copy link
Copy Markdown
Contributor Author

Replaced by #80141: #80141

Closing this copy so the active PR uses the hemant/ source branch prefix. Commit SHAs and author/committer identity are unchanged.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

app: web-ui App: web-ui docs Improvements or additions to documentation proof: sufficient ClawSweeper judged the real behavior proof convincing. proof: supplied External PR includes structured after-fix real behavior proof. size: M

Projects

None yet

Milestone

No milestone

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

@HemantSudarshan