fix(runtime): prompt permission mode never invokes the prompter#3274
Open
nankingjing wants to merge 1 commit into
Open
fix(runtime): prompt permission mode never invokes the prompter#3274nankingjing wants to merge 1 commit into
nankingjing wants to merge 1 commit into
Conversation
PermissionMode derives Ord from declaration order, placing Prompt (and Allow)
above the real privilege ladder (ReadOnly < WorkspaceWrite < DangerFullAccess).
In PermissionPolicy::authorize_with_context the ladder check
'current_mode >= required_mode' therefore evaluates true for every tool when the
active mode is Prompt (3 >= {0,1,2}), returning Allow and making the subsequent
'if current_mode == PermissionMode::Prompt' branch that routes to prompt_or_deny
unreachable.
Effect: a session configured with the 'prompt' permission mode silently
auto-allows every tool and never calls the prompter (reached via
ConversationRuntime::run_turn with an interactive prompter), defeating the mode's
entire purpose. The sibling PermissionEnforcer already special-cases Prompt
before its own 'active_mode >= required_mode' check, confirming Prompt is not
meant to be part of the ordered ladder.
Fix: exclude Prompt from the ordered-ladder comparison so Prompt mode falls
through to the interactive prompt path. Allow remains handled by its explicit
equality check. Adds regression tests covering Prompt mode with and without a
prompter.
|
Good catch. The Ord-derived ordering placing Prompt(3) above DangerFullAccess(2) means current_mode >= required_mode is always true for Prompt mode, making the interactive prompt path completely unreachable. The fix is minimal and correct — excluding Prompt from the ordered-ladder comparison so it falls through to prompt_or_deny as intended. The regression tests cover both the core fix (prompter is now invoked) and the edge case (non-Prompt modes unchanged). LGTM on the approach. |
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
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
promptpermission mode never actually prompts — it silently auto-allows every tool.PermissionModederivesOrdfrom declaration order:So
Promptsorts above the real privilege ladder (ReadOnly < WorkspaceWrite < DangerFullAccess). InPermissionPolicy::authorize_with_contextthe ladder check runs before the Prompt handling:required_mode_forreturns a configured requirement or defaults toDangerFullAccess, so in every normal configurationrequired_mode ∈ {ReadOnly, WorkspaceWrite, DangerFullAccess}. Whencurrent_mode == Prompt,current_mode >= required_modeis always true, the firstifreturnsAllow, and thecurrent_mode == PermissionMode::Promptbranch that routes toprompt_or_denyis dead code.Impact
ConversationRuntime::run_turncallsauthorize_with_context(..., Some(prompter))for each tool. A session configured with thepromptpermission mode is therefore never asked to approve tools — every tool is silently allowed and the prompter is never invoked, defeating the entire purpose of the mode.Why this is the intended-but-broken path
if current_mode == PermissionMode::Prompt { ... prompt_or_deny ... }branch shows the author intended Prompt mode to prompt within this function (it is the only path that accepts aprompter).PermissionEnforceralready special-casesPromptbefore its ownactive_mode >= required_modecheck (check,check_with_required_mode,check_bash,check_file_write), confirmingPromptis not meant to participate in the ordered ladder.authorize_with_contextsimply forgot to exclude it (it already excludesAllowvia an explicit==check).Fix
Exclude
Promptfrom the ordered-ladder comparison so Prompt mode falls through to the interactive prompt path:Allowis unaffected (still handled by its explicitcurrent_mode == PermissionMode::Allowcheck). Non-Prompt modes are unaffected (current_mode != Promptis true, so the comparison behaves exactly as before). ThePermissionEnforcerpath is unaffected because it never callsauthorizein Prompt mode.Tests
Adds two regression tests:
prompt_mode_routes_to_prompter_instead_of_auto_allowing— Prompt mode now invokes the prompter (seen.len() == 1) instead of auto-allowing.prompt_mode_denies_when_no_prompter_is_available— Prompt mode denies when no prompter is supplied.Both fail against the current code (which returns
Allowwithout touching the prompter).Verification
Verified by reading and by diffing the committed file against upstream; a full
cargo testwas not executed in this environment. The change is a single guard on one boolean sub-expression plus two additive tests that reuse existing test helpers (RecordingPrompter,PermissionRequest); no existing test constructs aPrompt-mode policy and callsauthorize, so no existing test changes behavior.