Skip to content

fix #97069: [Bug]: Approval prompt wrongly says effective policy requires approval for one-shot commands#97188

Closed
jincheng-xydt wants to merge 1 commit into
openclaw:mainfrom
jincheng-xydt:fix/approval-prompt-message-97069
Closed

fix #97069: [Bug]: Approval prompt wrongly says effective policy requires approval for one-shot commands#97188
jincheng-xydt wants to merge 1 commit into
openclaw:mainfrom
jincheng-xydt:fix/approval-prompt-message-97069

Conversation

@jincheng-xydt

Copy link
Copy Markdown
Contributor

Summary

  • Problem: When allow-always is unavailable because the command is one-shot (e.g., shell redirection 2>&1), the approval prompt misleadingly says "the effective approval policy requires approval every time" — even when the actual policy is ask=on-miss. This confuses users into thinking their policy is misconfigured ([Bug]: Approval prompt wrongly says effective policy requires approval every time for non-persistable redirected command #97069).
  • Solution: Check the ask setting in the message builder. If ask === "always", show the existing policy message. Otherwise, show a message explaining the command cannot be persisted.
  • What changed: exec-approval-reply.ts and exec-approval-forwarder.ts — conditional messages in 3 locations. Two new tests.
  • What did NOT change: no API, config, schema, or behavior changes. The allow-always button still correctly disappears for one-shot commands.

Maintainer checklist

  • Fix classification: Root-cause bug fix in exec approval message rendering
  • Maintainer-ready confidence: High — 56/56 tests pass

Real behavior proof

Behavior or issue addressed:
Approval prompt wrongly says "effective policy requires approval every time" for one-shot commands (shell redirection).

Real environment tested:

  • Platform: Linux x86_64
  • Node.js: v22.22.0
  • Runtime: Vitest v4.1.8
  • Branch: fix/approval-prompt-message-97069 (0e9b673)

Exact steps or command run after the patch:

node scripts/run-vitest.mjs src/infra/exec-approval-reply.test.ts src/infra/exec-approval-forwarder.test.ts --reporter=verbose

After-fix evidence:

 Test Files  2 passed (2)
      Tests  56 passed (56)
   Duration  8.42s

Observed result after fix:

  • ask=always: "The effective approval policy requires approval every time..." (unchanged) ✓
  • ask=on-miss + one-shot: "This command is not eligible for Allow Always (e.g., shell redirection cannot be persisted for reuse)" ✓
  • Forwarder: "Allow Always is unavailable because this command is not eligible for persistent reuse" ✓

What was not tested:

  • End-to-end exec approval flow with real gateway (requires live OpenClaw instance)

…ent in approval messages

resolveExecApprovalAllowedDecisions() removes allow-always from allowed
decisions when the command is not persistable (e.g., shell redirection),
but the user-facing message always said 'the effective policy requires
approval every time.' This misled users whose actual policy is ask=on-miss
into thinking their policy was misconfigured.

Fix: check params.ask (or request.request.ask) — if ask is 'always', show
the policy message. Otherwise, show a one-shot message explaining that the
command cannot be persisted.

Fixes openclaw#97069
@clawsweeper

clawsweeper Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

Thanks for the context here. I swept through the related work, and this is now duplicate or superseded.

Close in favor of the broader proof-positive sibling PR: this branch fixes two renderer messages, but it is still a partial landing candidate and is superseded by an open mergeable PR that covers the same approval-copy bug across production surfaces with sufficient Telegram proof.

Canonical path: Use #97077 as the canonical landing candidate after maintainer review, then close or supersede the narrower overlapping branches.

So I’m closing this here and keeping the remaining discussion on #97077.

Review details

Best possible solution:

Use #97077 as the canonical landing candidate after maintainer review, then close or supersede the narrower overlapping branches.

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

Yes. Current main source removes Allow Always for both ask=always and one-shot persistence, while the reported and Mantis baseline prompts show policy-only copy whenever Allow Always is absent.

Is this the best way to solve the issue?

No. This is a useful partial fix, but the best landing path is the broader proof-positive sibling branch that carries the needed context through production prompt surfaces and preserves the missing-ask fallback.

Security review:

Security review cleared: The diff changes approval prompt text and tests only; no dependency, workflow, secret, permission, package, or supply-chain surface changed.

AGENTS.md: found and applied where relevant.

What I checked:

Likely related people:

  • gumadeiras: Authored the effective exec approval reporting/actions change that added the generic unavailable Allow Always copy across shared approval surfaces. (role: introduced effective-policy approval prompt path; confidence: high; commits: ba735d015809; files: src/infra/exec-approval-reply.ts, src/infra/exec-approval-forwarder.ts, src/agents/bash-tools.exec-runtime.ts)
  • jesse-merhi: Authored the Tree-sitter command authorization planner work that introduced one-shot persistence and unavailable-decision behavior for unpersistable command shapes. (role: one-shot approval behavior contributor; confidence: high; commits: c9707ab635b9; files: src/infra/exec-approvals.ts, src/agents/bash-tools.exec-host-gateway.ts, src/agents/bash-tools.exec-host-node.ts)
  • steipete: Current blame and recent history show shared approval reply/forwarder lines carried through recent adjacent approval infrastructure work. (role: recent area contributor; confidence: medium; commits: 56259606d15f; files: src/infra/exec-approval-reply.ts, src/infra/exec-approval-forwarder.ts, src/infra/exec-approvals.ts)

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

@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. P2 Normal backlog priority with limited blast radius. labels Jun 27, 2026
@clawsweeper
clawsweeper Bot temporarily deployed to qa-live-shared June 27, 2026 06:30 Inactive
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. label Jun 27, 2026
@openclaw-mantis

Copy link
Copy Markdown
Contributor

Mantis Telegram Desktop Proof

Summary: Mantis captured native Telegram Desktop before/after GIFs showing the exec approval wording change for a one-shot command.

Main screenshot This PR screenshot
Baseline native Telegram Desktop screenshot Candidate native Telegram Desktop screenshot
Main This PR
Baseline native Telegram Desktop proof GIF Candidate native Telegram Desktop proof GIF

Motion-trimmed clips:

Raw QA files: https://artifacts.openclaw.ai/mantis/telegram-desktop/pr-97188/run-28281206148-1/index.json

@clawsweeper clawsweeper Bot added proof: sufficient ClawSweeper judged the real behavior proof convincing. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 27, 2026
@clawsweeper

clawsweeper Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper applied the proposed close for this PR.

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

Labels

mantis: telegram-visible-proof Mantis should capture Telegram visible proof. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal backlog priority with limited blast radius. proof: 📸 screenshot Contributor real behavior proof includes screenshot evidence. proof: sufficient ClawSweeper judged the real behavior proof convincing. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant