fix(infra): clarify Allow Always unavailable message for one-shot commands#98108
Open
yplongapp wants to merge 6 commits into
Open
fix(infra): clarify Allow Always unavailable message for one-shot commands#98108yplongapp wants to merge 6 commits into
yplongapp wants to merge 6 commits into
Conversation
Contributor
|
Codex review: needs real behavior proof before merge. Reviewed July 9, 2026, 8:19 AM ET / 12:19 UTC. Summary PR surface: Source +20, Tests +87. Total +107 across 16 files. Reproducibility: yes. The linked issue gives redirected-command steps and screenshots, and current source shows one-shot persistence and ask=always both remove Allow Always while renderers still need a reason signal. Review metrics: 2 noteworthy metrics.
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
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Land one canonical full-surface fix that preserves approval semantics and renders reason-specific Allow Always copy consistently across chat/gateway, Telegram, embedded/reaction, and Control UI surfaces, with current-head real prompt proof. Do we have a high-confidence way to reproduce the issue? Yes. The linked issue gives redirected-command steps and screenshots, and current source shows one-shot persistence and ask=always both remove Allow Always while renderers still need a reason signal. Is this the best way to solve the issue? No. Threading Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against f786efddcd88. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +20, Tests +87. Total +107 across 16 files. View PR surface stats
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
Review history (4 earlier review cycles)
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
Contributor
Mantis Telegram Desktop ProofSummary: Mantis captured native Telegram Desktop before/after GIFs showing the one-shot command approval prompt copy change.
Motion-trimmed clips: |
yplongapp
added a commit
to yplongapp/openclaw
that referenced
this pull request
Jun 30, 2026
…tinction The original PR openclaw#98108 only updated exec-approval-forwarder.ts and exec-approval-reply.ts to distinguish policy-level vs one-shot Allow Always messages. Several sibling surfaces still showed the misleading generic message for one-shot commands. - bash-tools.exec-runtime.ts: add ask param to buildApprovalPendingMessage, show one-shot message when ask != always - bash-tools.exec-host-shared.ts: add ask param to buildExecApprovalPendingToolResult, pass through to pending message - bash-tools.exec-host-gateway.ts: pass hostAsk to tool result - bash-tools.exec-host-node.ts: pass approvalDecisionAsk to tool result - approval-reaction-runtime.ts: add ask param to buildManualInstructionSection, pass view.ask for exec approvals - Add test coverage for one-shot messages in reply and forwarder tests
openclaw-barnacle
Bot
added
agents
Author
|
@clawsweeper re-review |
Contributor
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
proof: 📸 screenshot
Author
|
@clawsweeper re-review |
Contributor
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
status: 📣 needs proof
Author
|
@clawsweeper re-review |
Contributor
|
🦞🧹 I asked ClawSweeper to review this item again. |
Author
|
@clawsweeper re-review |
Author
|
@clawsweeper re-review: added missing Telegram native handler, Telegram forwarding, and embedded reconstruction paths. See commit 1e0baee. |
Contributor
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
rating: 🦪 silver shellfish
Author
|
@clawsweeper re-review: current head now includes Telegram native handler, Telegram forwarding, and embedded reconstruction paths (commit 1e0baee + 3c50f72) |
…mands When Allow Always is unavailable due to the command's shell redirection or runtime payload (not because the policy is ask=always), the approval prompt now explains the actual reason instead of incorrectly claiming "the effective approval policy requires approval every time". Closes openclaw#97069
…tinction The original PR openclaw#98108 only updated exec-approval-forwarder.ts and exec-approval-reply.ts to distinguish policy-level vs one-shot Allow Always messages. Several sibling surfaces still showed the misleading generic message for one-shot commands. - bash-tools.exec-runtime.ts: add ask param to buildApprovalPendingMessage, show one-shot message when ask != always - bash-tools.exec-host-shared.ts: add ask param to buildExecApprovalPendingToolResult, pass through to pending message - bash-tools.exec-host-gateway.ts: pass hostAsk to tool result - bash-tools.exec-host-node.ts: pass approvalDecisionAsk to tool result - approval-reaction-runtime.ts: add ask param to buildManualInstructionSection, pass view.ask for exec approvals - Add test coverage for one-shot messages in reply and forwarder tests
Extract the duplicated if/else pattern that selects the policy-level
vs one-shot Allow Always message into a single shared helper
formatAllowAlwaysUnavailableMessage(ask?) in exec-approvals.ts.
All 5 call sites now delegate to it, eliminating 3 redundant message
strings and 4 duplicate condition chains.
Previously:
forwarder: if (ask === 'always') { ... } else { ... }
reply: if (ask === 'always') { ... } else { ... }
exec-runtime: if (ask === 'always') { ... } else if (ask != null) { ... } else { ... }
reaction-sdk: if (ask === 'always') { ... } else if (ask != null) { ... } else { ... }
Now all four use:
formatAllowAlwaysUnavailableMessage(ask)
Also normalizes the two policy-always wordings into one canonical form:
"Allow Always is unavailable because the effective policy requires approval every time."
…approval validation The gateway RPC validateDecision callback had a hardcoded message that always said "effective policy requires approval every time" regardless of whether the actual reason was a one-shot command restriction. Route it through formatAllowAlwaysUnavailableMessage(ask) so it shows the correct reason like all other surfaces. Also fix the gateway test for unavailableDecisions to include ask=on-miss so it properly exercises the one-shot message path instead of falling back to the default ask value.
…nder paths ClawSweeper review identified 3 uncovered approval render paths that still lacked the ask signal needed by formatAllowAlwaysUnavailableMessage to choose the correct Allow Always unavailable message. 1. Native Telegram (extensions/telegram/src/approval-handler.runtime.ts): buildPendingPayload now passes view.ask to buildExecApprovalPendingReplyPayload. 2. Telegram forwarding (extensions/telegram/src/exec-approval-forwarding.ts): buildTelegramExecApprovalPendingPayload now passes request.ask. 3. Embedded reconstruction (src/agents/embedded-agent-subscribe.handlers.tools.ts): readExecApprovalPendingDetails now extracts ask from tool result details, and the call site passes it through. Also added ask field to ExecToolDetails approval-pending type and buildExecApprovalPendingToolResult details.
yplongapp
force-pushed
the
fix/exec-approval-misleading-message-97069
branch
from
July 9, 2026 11:58
3c50f72 to
cfc6c50
Compare
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
This pull request has been automatically marked as stale due to inactivity. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
openclaw --version 2>&1would show "The effective approval policy requires approval every time" even when the effective policy was ask=on-miss.ask === "always"before using the policy-related message; otherwise use the command-shape explanation.Root Cause
buildExecApprovalPendingReplyPayloadatsrc/infra/exec-approval-reply.ts:378used a single generic message wheneverallow-alwayswas not inallowedDecisions. The actual reason could be either (a) the policy isask=always, or (b) the command is one-shot (shell redirection / runtime payload prevents persistent approval). The message implied (a) in all cases, which was misleading.Verification
params.askis available at the reply payload builder;request.request.askis available at the forwarder message builder.ask === "always": preserves original accurate message.ask !== "always"(e.g. "on-miss", "off", null): uses the new command-shape explanation.exec-approval-reply.test.ts:319usesask: "always"and continues to pass.Real behavior proof
Behavior addressed: Approval prompt now shows accurate reason when Allow Always is unavailable for one-shot commands.
Environment tested: Source code analysis on OpenClaw main (Node 22, Linux).
Steps run after the patch: Traced the full approval-reply flow:
buildExecApprovalPendingReplyPayloadinsrc/infra/exec-approval-reply.ts: whenaskis not"always"andallow-alwaysis unavailable, uses the command-shape explanation.buildExecApprovalRequestMessageinsrc/infra/exec-approval-forwarder.ts: same differentiation viarequest.request.ask.Evidence after fix:
Observed result after the fix: Users running commands with shell redirection (
2>&1,2>/dev/null) no longer see a misleading message claiming the policy requires approval every time when the actual effective policy is ask=on-miss.Not tested: Live gateway session with real exec approval prompts. The change is a string-only message update with no behavioral logic change.