fix(approval): carry allow_permanent to TUI + desktop approval prompts#44534
Merged
Conversation
When a tirith content-security warning is present the approval backend forces allow_permanent=False and silently downgrades an "always" choice to session scope (the persistence loop in check_all_command_guards only honors "always" → permanent when no tirith finding exists). But the gateway notify payload that drives the TUI and the Electron desktop app never carried that flag, so both surfaces always rendered "Always allow" — offering a permanent allow the backend would quietly refuse to persist. Plumb allow_permanent end-to-end: - tools/approval.py: include `allow_permanent: not has_tirith` in the gateway approval_data the notify callback emits as `approval.request`. - ui-tui: thread `allowPermanent` through the event handler, gateway types, and ApprovalReq; ApprovalPrompt drops the "always" option (and renumbers the quick-pick keys) when it's false. - apps/desktop: thread `allow_permanent` through the gateway payload type, the per-session approval store, and the inline ApprovalBar, which now hides the "Always allow…" dropdown item when permanent allow is disallowed — reusing the existing DropdownMenu / confirm-Dialog UI. The desktop/TUI render path for approvals already landed in #38578 (the root cause of approvals not surfacing in the GUI); this completes the salvage of #37856 by carrying allow_permanent across both surfaces. #37856's original thread-local _block() approach is dropped: desktop/TUI approvals resolve via approval.respond → resolve_gateway_approval (the per-session queue), not the _block()/request_id correlation, so a worker-thread callback waiting on _block would never be released by the real UI. Tests: gateway notify payload carries allow_permanent (True without tirith, False with a tirith warning); ui-tui approvalAction reduced option set + event-handler allowPermanent propagation; desktop store round-trip + the ApprovalBar showing/hiding "Always allow". Supersedes #37856 Closes #37812 Co-authored-by: LeonSGP43 <cine.dreamer.one@gmail.com>
Contributor
🔎 Lint report:
|
… opt set Collapse the verbose multi-line rationale comments across the TUI/desktop/ backend approval surfaces into single-line "why" notes, and derive APPROVAL_OPTS_NO_ALWAYS from APPROVAL_OPTS instead of re-listing it. No behavior change.
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
2 participants
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
Salvages #37856. When a tirith content-security warning is present, the approval backend forces
allow_permanent=Falseand silently downgrades an "always" choice to session scope (the persistence loop incheck_all_command_guardsonly honors"always" → permanentwhen no tirith finding exists). But the gatewayapproval.requestpayload that drives the TUI and the Electron desktop app never carried that flag, so both surfaces always rendered "Always allow" — offering a permanent allow the backend would quietly refuse to persist.This plumbs
allow_permanentend-to-end:tools/approval.py— includeallow_permanent: not has_tirithin the gatewayapproval_datathe notify callback emits asapproval.request.allowPermanentthrough the event handler, gateway types, andApprovalReq;ApprovalPromptdrops the "always" option (and renumbers the quick-pick keys) when it's false.apps/desktop/— threadallow_permanentthrough the gateway payload type, the per-session approval store, and the inlineApprovalBar, which now hides the "Always allow…" dropdown item when permanent allow is disallowed — reusing the existingDropdownMenu/ confirm-DialogUI.Relationship to #37856 / #37812
allow_permanentacross both surfaces._block()approach is dropped on purpose: desktop/TUI approvals resolve viaapproval.respond → resolve_gateway_approval(the per-session queue), not the_block()/request_idcorrelation, so a worker-thread callback waiting on_blockwould never be released by the real UI.Co-authored-by.Test plan
tests/tools/test_command_guards.py(20/20) — new gateway-payload tests assertallow_permanentisTruewithout a tirith warning andFalsewith one.approvalActionreduced-option-set test + event-handlerallowPermanentpropagation (vitest, 80 passed);tsctypecheck clean.ApprovalBarshow/hide of "Always allow" (vitest --environment jsdom, 16 passed);tsctypecheck clean; eslint clean.Supersedes #37856
Closes #37812