fix(approvals): summarize long approval prompts#80141
Conversation
|
Codex review: needs real behavior proof before merge. Reviewed July 3, 2026, 11:52 PM ET / 03:52 UTC. Summary 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.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest 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:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 9d68f877ac3e. Label changesLabel justifications:
Evidence reviewedPR surface: Source +162, Tests +153, Docs +2. Total +317 across 14 files. View PR surface stats
Security concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
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
|
|
This pull request has been automatically marked as stale due to inactivity. |
|
This pull request has been automatically marked as stale due to inactivity. |
|
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. |
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-alwaysfrom 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 forask: "always"or other restricted approval modes.Architectural reasoning
src/infraand uses it only at presentation boundaries: fallback forwarded approval text, shared approval reply payloads, native approval view models, and the Control UI modal.systemRunPlan, bindings, and resolution path unchanged, so command identity and allow-always matching are not loosened.allowedDecisionsfor exec and plugin approval requests, then renders only those actions in the modal.Real behavior proof
hemant/58687-approval-prompt-summaryataaa32699bfff52d4147d2850f456eaa2c88f3dbe.src/infra/exec-approval-command-summary.tswithpnpm exec tsx.exec.approval.requestedpayload through the actual Control UI parser withallowedDecisions: ["allow-once", "deny"].includesLine8: false), reports the hidden content, and the parser returns onlyallow-onceanddeny, which is the decision set the modal will render.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.tspnpm 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.tspnpm lint:corepnpm docs:check-mdxpnpm lint:docspnpm tsgo:corevia temporaryP:\mapping because direct execution fromC:\Projects\Personal Project\openclawstill trips the path-with-spaces launcher issuepnpm tsgo:core:testvia temporaryP:\mappingpnpm tsgo:test:uivia temporaryP:\mappingpnpm check:changedgit diff --check upstream/main...HEADpnpm test:changedfanned out to the broadunit-fastshard and failed on unrelated Windows/local-environment assumptions: POSIX path expectation mismatches, table glyph fallback expectations, SQLite temp cleanupEBUSY, 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
allowedDecisionsin 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 summarizedview.commandTextfor 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
allowedDecisionsstill 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.