fix(approval): distinguish policy vs non-persistable reason when Allow Always unavailable#97145
fix(approval): distinguish policy vs non-persistable reason when Allow Always unavailable#97145SunnyShu0925 wants to merge 6 commits into
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 12, 2026, 10:42 PM ET / July 13, 2026, 02:42 UTC. Summary PR surface: Source +132, Tests +79. Total +211 across 59 files. Reproducibility: yes. at source and proof level: Review metrics: 2 noteworthy metrics.
Stored data model 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:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Rebase onto current Do we have a high-confidence way to reproduce the issue? Yes at source and proof level: Is this the best way to solve the issue? Yes, provisionally: reusing the already-carried effective AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 0e15a5d994a4. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +132, Tests +79. Total +211 across 59 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 (18 earlier review cycles; latest 8 shown)
|
02058fe to
66ae5f7
Compare
66ae5f7 to
fa5b811
Compare
fa5b811 to
7bf87f2
Compare
|
Rebased on latest This PR touches 57 files across 4 layers. If the size blocks review, I can split it into smaller, independently reviewable PRs:
Let me know if you prefer the split — happy to close this and open the smaller ones. |
|
I rebased this PR onto the latest
Feel free to pull my branch or let me know if I can help further. |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
…w Always unavailable
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
This PR has been tagged with @vincentkoc, when you have a moment, could you please take a look and review? Thanks! |
|
This PR has been superseded by #97740, which was merged by @vincentkoc on July 11, 2026 to fix the same issue (#97069). The two PRs took different approaches:
Since #97740 has already been merged and resolved the issue, closing this PR as superseded. Thank you @SunnyShu0925 for the contribution — the deeper approach may still be valuable reference if more granular unavailable-reason messaging is pursued in the future. |
|
Closed as superseded by #97740. |
Fixes #97069.
What Problem This Solves
Fixes an issue where users running commands with shell redirection (e.g.
openclaw --version 2>&1) would see "The effective approval policy requires approval every time, so Allow Always is unavailable" — even when their effective policy isask=on-miss. All nine approval prompt renderers (reply, forwarder, agent-facing, Control UI, reaction, embedded handler, Telegram handler, Telegram forwarding, and gateway API) blamed the policy instead of the real cause: the command is non-persistable and cannot be saved as an allow-always rule.Why This Change Was Made
resolveExecApprovalAllowedDecisions()returns the same["allow-once", "deny"]for bothask=always(policy) and one-shot persistence (non-persistable command). Downstream renderers had no way to distinguish the two cases.Rather than introducing a new typed protocol field (which previous attempts attempted across 65+ files), we thread the existing
askfield — already present in the request payload, approval view, and gateway pipeline — through the approval-pending data pipeline so renderers can branch on it. Approval policy logic is unchanged.The
nodehost path usesapprovalDecisionAsk(maxAsk(hostAsk, nodeAsk)) to keep the reasoning consistent withallowedDecisionscomputation even when the node policy is stricter than the host policy.User Impact
Users with
ask=on-miss(or any non-always policy) who run commands with shell redirection will now see:Instead of the misleading policy message. Users with
ask=alwayssee the same policy message as before. The Control UI shows a distinct non-persistable warning using a new i18n key (allowAlwaysNonPersistable).askinput"always""on-miss"(one-shot)"off"(shell redirect)Evidence
18 files, +109/−23 lines. Core logic is 52 lines; tests add 57 lines.
Renderers fixed (all 9): reply, forwarder, agent-facing, Control UI, reaction, embedded handler, Telegram handler, Telegram forwarding, gateway API error message.
Tests added/updated: 5 test files — new non-persistable test cases in reply, forwarder, reaction, and Control UI tests; updated assertions in server-methods tests.
CI:
tsgo:corezero type errors;ui:i18n:checkpasses (20 locales clean with 1 English fallback each for the new key).