-
Notifications
You must be signed in to change notification settings - Fork 37.8k
Enhance preToolUse hook and clean up code #293265
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
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.
Pull request overview
This PR enhances the preToolUse hook system by introducing a new 'ask' permission decision option, alongside the existing 'allow' and 'deny' options. The 'ask' decision forces user confirmation for tool execution even when auto-approval would normally apply. The implementation includes comprehensive test coverage and minor code cleanup.
Changes:
- Added 'ask' as a third permission decision for preToolUse hooks (along with 'allow' and 'deny')
- Implemented priority handling: deny > ask > allow
- Added
forceConfirmationReasonfield to tool invocation context to signal tools when confirmation is required - Updated hook execution logic to validate hookEventName and handle the new 'ask' decision
- Reorganized imports in hooksExecutionService.ts for better readability
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/vscode-dts/vscode.proposed.chatParticipantPrivate.d.ts | Added forceConfirmationReason optional field to LanguageModelToolInvocationPrepareOptions |
| src/vs/workbench/contrib/chat/common/hooks/hooksTypes.ts | Added 'ask' to PreToolUsePermissionDecision type with documentation |
| src/vs/workbench/contrib/chat/common/hooks/hooksExecutionService.ts | Implemented 'ask' handling with priority logic and hookEventName validation; reorganized imports |
| src/vs/workbench/contrib/chat/common/tools/languageModelToolsService.ts | Added forceConfirmationReason field and logic to handle 'ask' decision |
| src/vs/workbench/contrib/chat/browser/tools/languageModelToolsService.ts | Refactored preToolUse hook handling to return both denial and hook results; added resolveAutoConfirmFromHook method |
| src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts | Updated confirmation logic to respect forceConfirmationReason |
| src/vs/workbench/api/common/extHostLanguageModelTools.ts | Added forceConfirmationReason to extension host integration with API check |
| src/vs/workbench/contrib/chat/test/common/hooksExecutionService.test.ts | Added comprehensive tests for 'ask' decision and priority handling |
| src/vs/workbench/contrib/chat/test/browser/tools/languageModelToolsService.test.ts | Added integration tests for 'ask' and 'allow' decisions |
| chatSessionId?: string; | ||
| chatSessionResource: URI | undefined; | ||
| chatInteractionId?: string; | ||
| /** If set, tells the tool that it should include confirmmation messages. */ |
Copilot
AI
Feb 6, 2026
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.
Spelling error: "confirmmation" should be "confirmation".
* Flesh out preToolUse hook * Cleanup * cleanups * Cleanup
The changes improve the preToolUse hook by adding a new permission decision option and ensuring confirmation messages are displayed based on the context. Code cleanup enhances readability and maintainability.