-
Notifications
You must be signed in to change notification settings - Fork 2.4k
fix: auto-approval settings now properly apply during active tasks #8642
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Modified askApproval function in presentAssistantMessage.ts to check auto-approval settings before asking user - Added logic to determine which auto-approval setting applies based on tool type - Handle file path checks for inside/outside workspace auto-approval variants - Fixed TypeScript errors by properly handling different ClineAsk types Fixes #8641
| if ( | ||
| toolName === "readFile" || | ||
| toolName === "list_files" || | ||
| toolName === "search_files" || | ||
| toolName === "list_code_definition_names" | ||
| ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical Bug: Tool name mismatch will prevent auto-approval from ever working
The tool names checked here use camelCase (e.g., readFile, writeFile) but the actual tool names in this codebase use snake_case (e.g., read_file, write_to_file).
Evidence from the same file:
- Line 160:
case "execute_command": - Line 162:
case "read_file": - Line 171:
case "write_to_file": - Line 173:
case "apply_diff": - Line 196:
case "insert_content": - Line 198:
case "search_and_replace":
This means the conditions will never match, and auto-approval will never trigger. All tool names in the checks should use snake_case to match the actual tool names used throughout the codebase.
| if (toolInfo.isOutsideWorkspace) { | ||
| shouldAutoApprove = state.alwaysAllowReadOnlyOutsideWorkspace ?? false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic Error: toolInfo.isOutsideWorkspace property doesn't exist in the parsed JSON
The code checks toolInfo.isOutsideWorkspace to determine which auto-approval setting to use, but this property is never set in the JSON passed to askApproval.
Looking at how askApproval is called throughout the codebase, the partialMessage parameter is either:
- A tool description string like
"[read_file for 'path']"(fromtoolDescription()calls) - A simple JSON like
{"tool": "finishTask"}(fromaskFinishSubTaskApproval)
Neither includes an isOutsideWorkspace property. This means:
- Lines 302-306: The condition
if (toolInfo.isOutsideWorkspace)will never be true - Lines 318-320: The condition
else if (toolInfo.isOutsideWorkspace)will never be true
The workspace context needs to be determined differently, likely by examining the tool's parameters or the file path being operated on.
| if (toolInfo.isOutsideWorkspace) { | ||
| shouldAutoApprove = state.alwaysAllowReadOnlyOutsideWorkspace ?? false | ||
| } else { | ||
| shouldAutoApprove = state.alwaysAllowReadOnly ?? false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type Safety Issue: State properties used here are not defined in the state type
The code references several auto-approval properties on state that don't appear to be defined in the ProviderSettings type or related state types:
alwaysAllowReadOnlyalwaysAllowReadOnlyOutsideWorkspacealwaysAllowWritealwaysAllowWriteOutsideWorkspacealwaysAllowWriteProtectedalwaysAllowExecutealwaysAllowBrowseralwaysAllowFollowupQuestionsalwaysAllowUpdateTodoListalwaysAllowMcp
These properties should either:
- Be added to the appropriate type definitions (likely in
packages/types/src/provider-settings.tsor the state type) - Or be accessed from a different location if they exist elsewhere
Without these properties being properly typed and available in the state object, TypeScript may not catch errors and the feature won't work at runtime.
|
Close this one |
Description
This PR addresses Issue #8641 where auto-approval settings weren't being applied when toggled during an active task.
Changes
askApprovalfunction inpresentAssistantMessage.tsto check auto-approval settings before prompting the userClineAsktypesHow it Works
When a tool requests approval, the function now:
Testing
Note
The original issue reporter closed this as a 'false positive', but the fix could still be valuable for ensuring consistent auto-approval behavior. The implementation is ready for review and the team can decide whether to merge based on the actual need.
Fixes #8641
Important
Modifies
askApprovalinpresentAssistantMessage.tsto apply auto-approval settings based on tool type and context, fixing TypeScript errors.askApprovalinpresentAssistantMessage.tsto check auto-approval settings before user prompt.ClineAsktypes.This description was created by
for 6e92707. You can customize this summary. It will automatically update as commits are pushed.