-
Notifications
You must be signed in to change notification settings - Fork 3.1k
fix(computer-use): auto-approve install in auto-approve modes (YOLO/AUTO_EDIT/AUTO) #4756
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 |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ import { safeJsonStringify } from '../../utils/safeJsonStringify.js'; | |
| import { runBootstrap } from './bootstrap.js'; | ||
| import { isPackageSpecApproved, saveInstallState } from './install-state.js'; | ||
| import { resolveComputerUsePackageSpec } from './constants.js'; | ||
| import { ApprovalMode, type Config } from '../../config/config.js'; | ||
| import { homedir } from 'node:os'; | ||
|
|
||
| type ComputerUseParams = Record<string, unknown>; | ||
|
|
@@ -39,6 +40,7 @@ class ComputerUseInvocation extends BaseToolInvocation< | |
| constructor( | ||
| private readonly upstreamName: ComputerUseToolName, | ||
| params: ComputerUseParams, | ||
| private readonly config?: Config, | ||
| ) { | ||
| super(params); | ||
| } | ||
|
|
@@ -134,9 +136,21 @@ class ComputerUseInvocation extends BaseToolInvocation< | |
|
|
||
| // If the user confirmed through the pre-execution dialog, the install state | ||
| // was already written by onConfirm — runBootstrap will skip promptInstallApproval. | ||
| // For headless / SDK contexts (no dialog), fall back to the env-var path | ||
| // already built into bootstrap's default promptInstallApproval. | ||
| await runBootstrap(client, { signal, updateOutput }); | ||
| // But several approval modes auto-approve the tool call and bypass that | ||
|
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] The same YOLO/AUTO_EDIT/AUTO-bypasses-onConfirm narrative appears in four locations: this block, Consider keeping the full explanation in one authoritative location ( // See BootstrapContext.autoApproveInstall for the full rationale.— qwen3.7-max via Qwen Code /review
Collaborator
Author
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. Leaving the four comments as-is. They sit at four distinct layers (tool |
||
| // dialog entirely (so onConfirm never runs and install state is never | ||
| // written): YOLO (needsConfirmation() returns false), AUTO_EDIT | ||
| // (isAutoEditApproved() auto-approves info-type tools — all computer_use__* | ||
| // tools are info), and AUTO (classifier-approved calls). In those modes | ||
| // pass autoApproveInstall so the bootstrap honors the already-granted call | ||
| // approval instead of refusing with "install declined by user". DEFAULT | ||
| // still shows the dialog; PLAN blocks. Headless / SDK contexts (no config) | ||
| // fall back to the env-var path in bootstrap's default promptInstallApproval. | ||
| const mode = this.config?.getApprovalMode(); | ||
| const autoApproveInstall = | ||
|
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] The The PR's own history proves the coupling: the initial version only listed YOLO, and reviewers had to catch AUTO_EDIT and AUTO. When a new auto-approve mode is added, this file must be updated manually with no compile-time safety. Consider extracting a shared helper (e.g., — qwen3.7-max via Qwen Code /review
Collaborator
Author
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. Keeping the explicit enumeration. The duplication is intentional and fail-closed: a shared |
||
| mode === ApprovalMode.YOLO || | ||
| mode === ApprovalMode.AUTO_EDIT || | ||
| mode === ApprovalMode.AUTO; | ||
| await runBootstrap(client, { signal, updateOutput, autoApproveInstall }); | ||
|
|
||
| let mcpResult: CallToolResult; | ||
| try { | ||
|
|
@@ -182,6 +196,7 @@ export class ComputerUseTool extends BaseDeclarativeTool< | |
| constructor( | ||
| private readonly upstreamName: ComputerUseToolName, | ||
| schema: ComputerUseToolSchema, | ||
| private readonly config?: Config, | ||
| ) { | ||
| const qwenName = `computer_use__${upstreamName}`; | ||
| super( | ||
|
|
@@ -226,7 +241,7 @@ export class ComputerUseTool extends BaseDeclarativeTool< | |
| protected createInvocation( | ||
| params: ComputerUseParams, | ||
| ): ToolInvocation<ComputerUseParams, ToolResult> { | ||
| return new ComputerUseInvocation(this.upstreamName, params); | ||
| return new ComputerUseInvocation(this.upstreamName, params, this.config); | ||
| } | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.