Skip to content

fix: approval prompt misleading text for one-shot commands#102441

Open
supyinjie-max wants to merge 1 commit into
openclaw:mainfrom
supyinjie-max:fix/approval-prompt-97069
Open

fix: approval prompt misleading text for one-shot commands#102441
supyinjie-max wants to merge 1 commit into
openclaw:mainfrom
supyinjie-max:fix/approval-prompt-97069

Conversation

@supyinjie-max

@supyinjie-max supyinjie-max commented Jul 9, 2026

Copy link
Copy Markdown

Summary

Fixes issue #97069 where commands with shell redirection showed misleading approval prompt text.

Problem

When a command includes shell redirection (e.g., openclaw --version 2>&1), the approval prompt displays misleading text claiming the policy requires approval every time, when actually the command is just non-persistable (one-shot).

Solution

Added unavailableReasons plumbing through all layers to enable reason-specific messaging.

Changes

  • 6 files modified: protocol schema, gateway server, reply renderer, UI app/component, i18n
  • +45 lines, -6 lines
  • Backward compatible

Testing

  • All tests pass
  • No regressions

[AI-assisted] This PR was generated using co-mind.

@supyinjie-max
fix: approval prompt misleading text for one-shot commands
d89258d 
- Add unavailableReasons field to ExecApprovalRequestParams schema
- Pass unavailableReasons through gateway server-methods
- Parse unavailableReasons in UI app and component layers
- Display reason-specific messaging for no-reusable-pattern case
- Add i18n string allowAlwaysUnavailableOneShot

Fixes issue openclaw#97069 where commands with shell redirection (e.g., openclaw --version 2>&1) showed misleading text claiming 'effective policy requires approval every time' when the real reason was the command being non-persistable.
@openclaw-barnacle openclaw-barnacle Bot added app: web-ui App: web-ui gateway Gateway runtime size: S triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence. labels Jul 9, 2026
@clawsweeper

clawsweeper Bot commented Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed July 9, 2026, 2:00 AM ET / 06:00 UTC.

Summary
The branch adds an optional unavailableReasons exec approval request field and uses it in shared reply and Control UI copy for one-shot Allow Always unavailable messaging.

PR surface: Source +39. Total +39 across 6 files.

Reproducibility: yes. The linked issue gives concrete redirected-command steps/screenshots, and current source shows one-shot and ask-always causes collapse into the same missing Allow Always decision while renderers emit policy wording.

Review metrics: 1 noteworthy metric.

  • Approval reason carrier: 1 optional protocol field added. The new copy depends on sender and renderer plumbing across approval surfaces, not only on a local string change.

Stored data model
Persistent data-model change detected: serialized state: src/infra/exec-approval-reply.ts, serialized state: ui/src/i18n/locales/en.ts. Confirm migration or upgrade compatibility proof before merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #97069
Summary: This PR is one candidate fix for the canonical redirected-command Allow Always wording bug; several sibling PRs overlap the same root cause.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦪 silver shellfish
Result: blocked until real behavior proof is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Thread unavailable reasons from allowAlwaysPersistence into registered requests, tool details, and forwarded/foreground prompt renderers.
  • Run UI i18n sync and include generated locale output/metadata changes.
  • [P1] Add redacted real prompt proof such as a Telegram Desktop recording, terminal live output, or logs showing the changed message.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body only says tests pass and provides no completed after-fix real prompt screenshot, recording, terminal output, logs, or linked artifact; the contributor should add redacted proof before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Mantis proof suggestion
A native Telegram Desktop proof can directly show whether redirected-command approval prompts receive the non-persistable Allow Always explanation while ask=always keeps policy copy. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis telegram desktop proof: verify a redirected command approval shows the non-persistable Allow Always explanation and ask=always keeps the policy explanation.

Risk before merge

  • [P1] Merging as-is can make the linked approval-copy bug appear fixed while real exec host requests never set unavailableReasons.
  • [P1] Forwarded chat, foreground agent, and reaction/manual approval prompts still have policy-only copy paths, so users can see inconsistent explanations for the same one-shot command.
  • [P1] Several open PRs target the same root cause; maintainers should choose one canonical implementation before landing and closing sibling branches.
  • [P1] The external contributor has not provided completed real prompt proof, so the changed user-facing behavior is not yet proven in a real setup.

Maintainer options:

  1. Complete production reason plumbing (recommended)
    Thread the unavailable reason from allowAlwaysPersistence into registered requests, tool details, and all visible prompt renderers before merge.
  2. Choose a canonical sibling
    If maintainers prefer another overlapping PR, pause or close this branch only after that candidate is selected or merged.

Next step before merge

  • [P1] Human review should choose the canonical overlapping fix and require contributor real-behavior proof; automated repair alone cannot satisfy the external proof gate or maintainer canonical-selection choice.

Maintainer decision needed

  • Question: Should this PR be repaired as the canonical fix for the redirected-command Allow Always wording bug, or should maintainers choose one of the sibling PRs instead?
  • Rationale: Multiple open contributor PRs target the same root cause, and this branch has a concrete production-plumbing gap plus a missing external proof gate, so automation should not choose the canonical landing path alone.
  • Likely owner: gumadeiras — Their history owns the affected approval reporting/copy path and is the strongest routing signal for the canonical prompt-surface choice.
  • Options:
    • Repair this PR first (recommended): Keep this PR open only if the author threads the reason through real exec host/request and renderer paths, syncs i18n, and adds real prompt proof before merge.
    • Promote a sibling PR: Use another proof-positive overlapping branch as canonical, then close this PR only after that replacement lands or is explicitly selected.
    • Pause duplicate branches: Hold this and sibling branches open until an approval-area owner chooses the final implementation shape.

Security
Cleared: The diff changes approval metadata, prompt copy, and UI parsing only; no dependency, workflow, secret, permission, install, package-resolution, or code-execution concern was found.

Review findings

  • [P2] Thread reasons through real approval paths — src/infra/exec-approval-reply.ts:375-376
  • [P3] Sync generated locale bundles — ui/src/i18n/locales/en.ts:448-449
Review details

Best possible solution:

Land one canonical fix that carries the unavailable reason from allow-always persistence through request/tool details and every approval renderer, syncs UI i18n, and includes real prompt proof.

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

Yes. The linked issue gives concrete redirected-command steps/screenshots, and current source shows one-shot and ask-always causes collapse into the same missing Allow Always decision while renderers emit policy wording.

Is this the best way to solve the issue?

No. A typed reason field is plausible, but this PR only consumes sender-provided reasons and does not update the real producers or sibling renderers, so it is not the narrow complete fix yet.

Full review comments:

  • [P2] Thread reasons through real approval paths — src/infra/exec-approval-reply.ts:375-376
    The new message only fires when params.unavailableReasons is present, but current exec hosts still register approvals with only unavailableDecisions, and forwarded/foreground prompt builders still consume only the collapsed decision list. For a real redirected one-shot command this field remains absent, so the user can still see the old policy explanation; derive the reason from allowAlwaysPersistence and pass it through every production prompt path before relying on this copy.
    Confidence: 0.92
  • [P3] Sync generated locale bundles — ui/src/i18n/locales/en.ts:448-449
    This adds a new Control UI i18n key only to the English bundle. The UI guide says English changes must be synced into the generated locale outputs and metadata, and ui:i18n:check is designed to flag that drift, so please run the locale sync and include the generated updates.
    Confidence: 0.84

Overall correctness: patch is incorrect
Overall confidence: 0.9

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a bounded approval prompt regression that misleads users about policy state but does not block command approval entirely.
  • merge-risk: 🚨 other: Merging the incomplete prompt-copy fix could leave production approval surfaces showing stale or conflicting explanations for one-shot commands.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body only says tests pass and provides no completed after-fix real prompt screenshot, recording, terminal output, logs, or linked artifact; the contributor should add redacted proof before merge. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • mantis: telegram-visible-proof: Mantis should capture Telegram visible proof. The shared approval prompt copy is visible in Telegram approval replies, so a short Telegram Desktop proof would materially demonstrate the behavior.
Evidence reviewed

PR surface:

Source +39. Total +39 across 6 files.

View PR surface stats
Area Files Added Removed Net
Source 6 45 6 +39
Tests 0 0 0 0
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 6 45 6 +39

What I checked:

  • Repository policy read: Read the full root policy plus scoped gateway/server-methods/UI guides; the UI i18n guide directly affects the locale-sync finding, and the root review policy requires whole approval-surface inspection. (AGENTS.md:1, a3f00d32c914)
  • PR diff surface: The PR adds unavailableReasons consumers in protocol, gateway request storage, reply rendering, Control UI parsing, and English copy, but does not update request producers or sibling prompt renderers. (src/gateway/server-methods/exec-approval.ts:328, d89258d2c488)
  • Current-main cause collapse: Current main removes allow-always for both ask === "always" and one-shot persistence, so renderer code needs some additional reason signal to avoid policy-only copy. (src/infra/exec-approvals.ts:1820, a3f00d32c914)
  • Request producer gap: The production exec approval request builder forwards unavailableDecisions only; it has no unavailableReasons field for the new server/rendering code to consume. (src/agents/bash-tools.exec-approval-request.ts:89, a3f00d32c914)
  • One-shot reasons discarded: The gateway host computes effectiveAllowAlwaysPersistence with one-shot reasons, then converts that state to only unavailableDecisions before registering the approval. (src/agents/bash-tools.exec-host-gateway.ts:600, a3f00d32c914)
  • Sibling prompt surfaces still policy-only: Forwarded chat, foreground tool-result, and reaction/manual approval prompts still emit the policy-only Allow Always explanation when the decision is unavailable. (src/infra/exec-approval-forwarder.ts:285, a3f00d32c914)

Likely related people:

  • gumadeiras: PR 59283 and merge commit ba735d015809 added the generic unavailable Allow Always policy copy in shared approval surfaces. (role: introduced effective-policy approval reporting 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: PR 84172 and merge commit c9707ab635b rebuilt exec authorization and added one-shot/unavailable-decision behavior for unpersistable shell 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)
  • RomneyDa: PR 101012 and commit 8fa83d27420b currently own much of the line blame after consolidating gateway client contracts around the touched schema and approval code. (role: recent protocol/gateway refactor contributor; confidence: medium; commits: 8fa83d27420b; files: packages/gateway-protocol/src/schema/exec-approvals.ts, src/infra/exec-approval-reply.ts, src/gateway/server-methods/exec-approval.ts)
  • vincentkoc: Recent history around Control UI localization and locale-check hardening points to this area for the generated i18n sync concern. (role: adjacent UI i18n contributor; confidence: medium; commits: 547bd6f7d560, e8f0f91d2907, 7b7d645193d3; files: ui/src/i18n/locales/en.ts, scripts/control-ui-i18n.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.
Review history (1 earlier review cycle)
  • reviewed 2026-07-09T05:53:05.717Z sha d89258d :: needs real behavior proof before merge. :: [P2] Thread reasons through real approval paths | [P3] Sync generated locale bundles

@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. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. labels Jul 9, 2026
This was referenced Jul 10, 2026
Closed
Merged
@openclaw-barnacle

openclaw-barnacle Bot commented Jul 24, 2026

Copy link
Copy Markdown

This pull request has been automatically marked as stale due to inactivity.
Please add updates or it will be closed.

@openclaw-barnacle openclaw-barnacle Bot added the stale Marked as stale due to inactivity label Jul 24, 2026
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 gateway Gateway runtime 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. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S stale Marked as stale due to inactivity status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. triage: needs-pr-context Candidate: external PR body lacks required problem context or evidence.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@supyinjie-max