Skip to content

fix(runtime): prompt permission mode never invokes the prompter#3274

Open
nankingjing wants to merge 1 commit into
ultraworkers:mainfrom
nankingjing:fix-prompt-mode-never-prompts
Open

fix(runtime): prompt permission mode never invokes the prompter#3274
nankingjing wants to merge 1 commit into
ultraworkers:mainfrom
nankingjing:fix-prompt-mode-never-prompts

Conversation

@nankingjing

Copy link
Copy Markdown

Summary

prompt permission mode never actually prompts — it silently auto-allows every tool.

PermissionMode derives Ord from declaration order:

pub enum PermissionMode {
    ReadOnly,          // 0
    WorkspaceWrite,    // 1
    DangerFullAccess,  // 2
    Prompt,            // 3
    Allow,             // 4
}

So Prompt sorts above the real privilege ladder (ReadOnly < WorkspaceWrite < DangerFullAccess). In PermissionPolicy::authorize_with_context the ladder check runs before the Prompt handling:

if allow_rule.is_some()
    || current_mode == PermissionMode::Allow
    || current_mode >= required_mode      // <-- Prompt(3) >= {0,1,2} is always true
{
    return PermissionOutcome::Allow;
}

if current_mode == PermissionMode::Prompt   // <-- unreachable in normal configs
    || (current_mode == PermissionMode::WorkspaceWrite
        && required_mode == PermissionMode::DangerFullAccess)
{
    // ... route to prompt_or_deny ...
}

required_mode_for returns a configured requirement or defaults to DangerFullAccess, so in every normal configuration required_mode ∈ {ReadOnly, WorkspaceWrite, DangerFullAccess}. When current_mode == Prompt, current_mode >= required_mode is always true, the first if returns Allow, and the current_mode == PermissionMode::Prompt branch that routes to prompt_or_deny is dead code.

Impact

ConversationRuntime::run_turn calls authorize_with_context(..., Some(prompter)) for each tool. A session configured with the prompt permission 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

  • The dead 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 a prompter).
  • The sibling PermissionEnforcer already special-cases Prompt before its own active_mode >= required_mode check (check, check_with_required_mode, check_bash, check_file_write), confirming Prompt is not meant to participate in the ordered ladder. authorize_with_context simply forgot to exclude it (it already excludes Allow via an explicit == check).

Fix

Exclude Prompt from the ordered-ladder comparison so Prompt mode falls through to the interactive prompt path:

    || (current_mode != PermissionMode::Prompt && current_mode >= required_mode)

Allow is unaffected (still handled by its explicit current_mode == PermissionMode::Allow check). Non-Prompt modes are unaffected (current_mode != Prompt is true, so the comparison behaves exactly as before). The PermissionEnforcer path is unaffected because it never calls authorize in 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 Allow without touching the prompter).

Verification

Verified by reading and by diffing the committed file against upstream; a full cargo test was 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 a Prompt-mode policy and calls authorize, so no existing test changes behavior.

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.
@1716775457damn

Copy link
Copy Markdown

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants