-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(core): keep the ask_user_question dialog behind allow rules and auto-approval #10160
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -20,7 +20,7 @@ import type { FunctionDeclaration } from '@google/genai'; | |||||||||||||||||||
| import type { Config } from '../config/config.js'; | ||||||||||||||||||||
| import { ToolDisplayNames, ToolNames } from './tool-names.js'; | ||||||||||||||||||||
| import { createDebugLogger } from '../utils/debugLogger.js'; | ||||||||||||||||||||
| import { InputFormat } from '../output/types.js'; | ||||||||||||||||||||
| import { resolveInteractionMode } from '../core/prompts.js'; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const debugLogger = createDebugLogger('ASK_USER_QUESTION'); | ||||||||||||||||||||
|
|
||||||||||||||||||||
|
|
@@ -171,23 +171,44 @@ class AskUserQuestionToolInvocation extends BaseToolInvocation< | |||||||||||||||||||
| return `Ask user ${questionCount} question${questionCount > 1 ? 's' : ''}`; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
| * Whether a host is present that can put the questions in front of the | ||||||||||||||||||||
| * user. ACP hosts (VSCode extension, Zed, stream-json clients) run in | ||||||||||||||||||||
| * non-interactive mode but still collect answers through the | ||||||||||||||||||||
| * confirmation channel. | ||||||||||||||||||||
| */ | ||||||||||||||||||||
| private canCollectAnswers(): boolean { | ||||||||||||||||||||
| return resolveInteractionMode(this._config) !== 'headless'; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
Comment on lines
+180
to
+182
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion]
Suggested change
If you apply this, run the 中文说明
若应用该修改,请把 — qwen3.8-max via Qwen Code /review (v0.22.2) |
||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
| * ask_user_question always requires user confirmation so the user can | ||||||||||||||||||||
| * provide answers. In non-interactive mode without ACP support, we skip | ||||||||||||||||||||
| * confirmation (and subsequently skip execution). | ||||||||||||||||||||
| */ | ||||||||||||||||||||
| override async getDefaultPermission(): Promise<PermissionDecision> { | ||||||||||||||||||||
| const isAcpMode = | ||||||||||||||||||||
| this._config.getExperimentalZedIntegration() || | ||||||||||||||||||||
| this._config.getInputFormat() === InputFormat.STREAM_JSON; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| if (!this._config.isInteractive() && !isAcpMode) { | ||||||||||||||||||||
| if (!this.canCollectAnswers()) { | ||||||||||||||||||||
| // Non-interactive + no ACP: skip entirely | ||||||||||||||||||||
| return 'allow'; | ||||||||||||||||||||
| } | ||||||||||||||||||||
| return 'ask'; | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| /** | ||||||||||||||||||||
| * The confirmation dialog IS this tool: the answers are collected through | ||||||||||||||||||||
| * `onConfirm`, so an approval that skips the dialog does not "allow" the | ||||||||||||||||||||
| * tool, it silently answers "declined" on the user's behalf. Permission | ||||||||||||||||||||
| * rules and automatic approval modes must therefore never satisfy it — | ||||||||||||||||||||
| * a bare `ask_user_question` allow rule (a skill's `allowedTools` grant, | ||||||||||||||||||||
| * `permissions.allow`, an "always allow" answer) would otherwise override | ||||||||||||||||||||
| * the 'ask' default at L4 and the scheduler would run the tool with no | ||||||||||||||||||||
| * dialog ever shown. Headless runs stay as they were: nothing can prompt | ||||||||||||||||||||
| * there, and `execute()` reports that instead. | ||||||||||||||||||||
| */ | ||||||||||||||||||||
| override requiresUserInteraction(): boolean { | ||||||||||||||||||||
| return this.canCollectAnswers(); | ||||||||||||||||||||
| } | ||||||||||||||||||||
|
|
||||||||||||||||||||
| override async getConfirmationDetails( | ||||||||||||||||||||
| _abortSignal: AbortSignal, | ||||||||||||||||||||
| ): Promise<ToolAskUserQuestionConfirmationDetails> { | ||||||||||||||||||||
|
|
@@ -222,14 +243,8 @@ class AskUserQuestionToolInvocation extends BaseToolInvocation< | |||||||||||||||||||
|
|
||||||||||||||||||||
| async execute(_signal: AbortSignal): Promise<ToolResult> { | ||||||||||||||||||||
| try { | ||||||||||||||||||||
| // Check if we're in a mode that supports user interaction | ||||||||||||||||||||
| // ACP mode (VSCode extension, etc.) uses non-interactive mode but can still collect user input | ||||||||||||||||||||
| const isAcpMode = | ||||||||||||||||||||
| this._config.getExperimentalZedIntegration() || | ||||||||||||||||||||
| this._config.getInputFormat() === InputFormat.STREAM_JSON; | ||||||||||||||||||||
|
|
||||||||||||||||||||
| // In non-interactive mode without ACP support, we cannot collect user input | ||||||||||||||||||||
| if (!this._config.isInteractive() && !isAcpMode) { | ||||||||||||||||||||
| if (!this.canCollectAnswers()) { | ||||||||||||||||||||
| const errorMessage = | ||||||||||||||||||||
| 'Cannot ask user questions in non-interactive mode without ACP support. Please run in interactive mode or enable ACP mode to use this tool.'; | ||||||||||||||||||||
| return { | ||||||||||||||||||||
|
|
||||||||||||||||||||
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.
[Suggestion] This name check is the whole fix of the stream-json answer-drop regression — the unscoped
if (requiresUserInteraction)at the base commit confirmed with no payload, silently discarding the answers the SDK host had just collected — yet nothing in the code says whyexit_plan_modealone takes the payload-less branch. The rationale currently lives only in two tests, so a future maintainer "simplifying" the seemingly redundanttoolCall.request.name === ToolNames.EXIT_PLAN_MODEcondition re-broadens the guard and drops stream-json answers again — the exact regression this PR fixes. Conversely, someone adding a third outcome-only interactive tool has no in-code signal about which path it must take, and hostupdatedInputcan silently overwrite the args the user approved. A short comment pins the contract:中文说明
这个工具名判断正是 stream-json 回答丢失回归的全部修复——base 提交上未加限定的
if (requiresUserInteraction)会不带 payload 直接确认,悄悄丢掉 SDK 宿主刚收集到的答案——但代码里没有任何地方说明为什么只有exit_plan_mode走这条不带 payload 的分支。理由目前只存在于两个测试里:未来某位维护者"简化"这个看似冗余的toolCall.request.name === ToolNames.EXIT_PLAN_MODE条件,就会重新放宽守卫、再次丢掉 stream-json 的答案——正是本 PR 修复的回归。反过来,若有人新增第三个"只要结果"的交互工具,也没有任何代码内信号告诉它该走哪条路径,宿主的updatedInput可能悄悄覆盖用户批准的参数。一条简短注释即可锁定该约定。— qwen3.8-max via Qwen Code /review (v0.22.2)