refactor(core): replace positional execute params with ExecuteOptions bag (#22674)

This commit is contained in:
Adam Weidman
2026-03-16 17:50:24 -04:00
committed by GitHub
parent 990d010ecf
commit 605432ea70
10 changed files with 50 additions and 50 deletions

View File

@@ -51,10 +51,9 @@ class MockBackgroundableInvocation extends BaseToolInvocation<
async execute(
_signal: AbortSignal,
_updateOutput?: (output: ToolLiveOutput) => void,
_shellExecutionConfig?: unknown,
setExecutionIdCallback?: (executionId: number) => void,
options?: { setExecutionIdCallback?: (executionId: number) => void },
) {
setExecutionIdCallback?.(4242);
options?.setExecutionIdCallback?.(4242);
return {
llmContent: 'pid',
returnDisplay: 'pid',
@@ -111,7 +110,6 @@ describe('executeToolWithHooks', () => {
mockTool,
undefined,
undefined,
undefined,
mockConfig,
);
@@ -136,7 +134,6 @@ describe('executeToolWithHooks', () => {
mockTool,
undefined,
undefined,
undefined,
mockConfig,
);
@@ -168,7 +165,6 @@ describe('executeToolWithHooks', () => {
mockTool,
undefined,
undefined,
undefined,
mockConfig,
);
@@ -200,7 +196,6 @@ describe('executeToolWithHooks', () => {
mockTool,
undefined,
undefined,
undefined,
mockConfig,
);
@@ -234,7 +229,6 @@ describe('executeToolWithHooks', () => {
mockTool,
undefined,
undefined,
undefined,
mockConfig,
);
@@ -275,7 +269,6 @@ describe('executeToolWithHooks', () => {
mockTool,
undefined,
undefined,
undefined,
mockConfig,
);
@@ -298,8 +291,7 @@ describe('executeToolWithHooks', () => {
abortSignal,
mockTool,
undefined,
undefined,
setExecutionIdCallback,
{ setExecutionIdCallback },
mockConfig,
);

View File

@@ -11,10 +11,10 @@ import type {
AnyDeclarativeTool,
AnyToolInvocation,
ToolLiveOutput,
ExecuteOptions,
} from '../tools/tools.js';
import { ToolErrorType } from '../tools/tool-error.js';
import { debugLogger } from '../utils/debugLogger.js';
import type { ShellExecutionConfig } from '../index.js';
import { DiscoveredMCPToolInvocation } from '../tools/mcp-tool.js';
/**
@@ -61,8 +61,7 @@ function extractMcpContext(
* @param toolName The name of the tool
* @param signal Abort signal for cancellation
* @param liveOutputCallback Optional callback for live output updates
* @param shellExecutionConfig Optional shell execution config
* @param setExecutionIdCallback Optional callback to set an execution ID for backgroundable invocations
* @param options Optional execution options (shell config, execution ID callback, etc.)
* @param config Config to look up MCP server details for hook context
* @returns The tool result
*/
@@ -72,8 +71,7 @@ export async function executeToolWithHooks(
signal: AbortSignal,
tool: AnyDeclarativeTool,
liveOutputCallback?: (outputChunk: ToolLiveOutput) => void,
shellExecutionConfig?: ShellExecutionConfig,
setExecutionIdCallback?: (executionId: number) => void,
options?: ExecuteOptions,
config?: Config,
originalRequestName?: string,
): Promise<ToolResult> {
@@ -158,8 +156,7 @@ export async function executeToolWithHooks(
const toolResult: ToolResult = await invocation.execute(
signal,
liveOutputCallback,
shellExecutionConfig,
setExecutionIdCallback,
options,
);
// Append notification if parameters were modified

View File

@@ -156,6 +156,12 @@ export * from './services/executionLifecycleService.js';
// Export Injection Service
export * from './config/injectionService.js';
// Export Execution Lifecycle Service
export * from './services/executionLifecycleService.js';
// Export Injection Service
export * from './config/injectionService.js';
// Export base tool definitions
export * from './tools/tools.js';
export * from './tools/tool-error.js';

View File

@@ -570,14 +570,13 @@ describe('ToolExecutor', () => {
_sig,
_tool,
_liveCb,
_shellCfg,
setExecutionIdCallback,
options,
_config,
_originalRequestName,
) => {
// Simulate the tool reporting an execution ID
if (setExecutionIdCallback) {
setExecutionIdCallback(testPid);
if (options?.setExecutionIdCallback) {
options.setExecutionIdCallback(testPid);
}
return { llmContent: 'done', returnDisplay: 'done' };
},
@@ -624,16 +623,8 @@ describe('ToolExecutor', () => {
const testExecutionId = 67890;
vi.mocked(coreToolHookTriggers.executeToolWithHooks).mockImplementation(
async (
_inv,
_name,
_sig,
_tool,
_liveCb,
_shellCfg,
setExecutionIdCallback,
) => {
setExecutionIdCallback?.(testExecutionId);
async (_inv, _name, _sig, _tool, _liveCb, options) => {
options?.setExecutionIdCallback?.(testExecutionId);
return { llmContent: 'done', returnDisplay: 'done' };
},
);

View File

@@ -112,8 +112,7 @@ export class ToolExecutor {
signal,
tool,
liveOutputCallback,
shellExecutionConfig,
setExecutionIdCallback,
{ shellExecutionConfig, setExecutionIdCallback },
this.config,
request.originalRequestName,
);

View File

@@ -22,13 +22,13 @@ import {
type ToolExecuteConfirmationDetails,
type PolicyUpdateOptions,
type ToolLiveOutput,
type ExecuteOptions,
} from './tools.js';
import { getErrorMessage } from '../utils/errors.js';
import { summarizeToolOutput } from '../utils/summarizer.js';
import {
ShellExecutionService,
type ShellExecutionConfig,
type ShellOutputEvent,
} from '../services/shellExecutionService.js';
import { formatBytes } from '../utils/formatters.js';
@@ -150,9 +150,9 @@ export class ShellToolInvocation extends BaseToolInvocation<
async execute(
signal: AbortSignal,
updateOutput?: (output: ToolLiveOutput) => void,
shellExecutionConfig?: ShellExecutionConfig,
setExecutionIdCallback?: (executionId: number) => void,
options?: ExecuteOptions,
): Promise<ToolResult> {
const { shellExecutionConfig, setExecutionIdCallback } = options ?? {};
const strippedCommand = stripShellWrapper(this.params.command);
if (signal.aborted) {

View File

@@ -22,6 +22,15 @@ import {
import { type ApprovalMode } from '../policy/types.js';
import type { SubagentProgress } from '../agents/types.js';
/**
* Options bag for tool execution, replacing positional parameters that are
* only relevant to specific tool types.
*/
export interface ExecuteOptions {
shellExecutionConfig?: ShellExecutionConfig;
setExecutionIdCallback?: (executionId: number) => void;
}
/**
* Represents a validated and ready-to-execute tool call.
* An instance of this is created by a `ToolBuilder`.
@@ -68,8 +77,7 @@ export interface ToolInvocation<
execute(
signal: AbortSignal,
updateOutput?: (output: ToolLiveOutput) => void,
shellExecutionConfig?: ShellExecutionConfig,
setExecutionIdCallback?: (executionId: number) => void,
options?: ExecuteOptions,
): Promise<TResult>;
/**
@@ -325,7 +333,7 @@ export abstract class BaseToolInvocation<
abstract execute(
signal: AbortSignal,
updateOutput?: (output: ToolLiveOutput) => void,
shellExecutionConfig?: ShellExecutionConfig,
options?: ExecuteOptions,
): Promise<TResult>;
}
@@ -522,10 +530,10 @@ export abstract class DeclarativeTool<
params: TParams,
signal: AbortSignal,
updateOutput?: (output: ToolLiveOutput) => void,
shellExecutionConfig?: ShellExecutionConfig,
options?: ExecuteOptions,
): Promise<TResult> {
const invocation = this.build(params);
return invocation.execute(signal, updateOutput, shellExecutionConfig);
return invocation.execute(signal, updateOutput, options);
}
/**