From 68d00feb629a997731f8a300056daf6d69d768ab Mon Sep 17 00:00:00 2001 From: mkorwel Date: Thu, 19 Mar 2026 16:32:46 -0700 Subject: [PATCH] fix(core): Resolve context initialization mismatch in built-in tools - Update ShellTool, WebFetchTool, and WebSearchTool to handle both Config and AgentLoopContext. - Add safety checks for config in ToolConfirmationMessage UI. - Add safety checks for context.config in policy update logic. - Fixes #23174 --- .../messages/ToolConfirmationMessage.tsx | 5 +- packages/core/src/scheduler/policy.ts | 7 +- packages/core/src/tools/shell.ts | 65 +++++++++++------- packages/core/src/tools/web-fetch.ts | 51 +++++++++----- packages/core/src/tools/web-search.ts | 26 +++++-- plans/implemented/fix-trusted-folder-error.md | 67 +++++++++++++++++++ 6 files changed, 170 insertions(+), 51 deletions(-) create mode 100644 plans/implemented/fix-trusted-folder-error.md diff --git a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx index 45584a9d46..be85d8ba73 100644 --- a/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx +++ b/packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx @@ -88,13 +88,14 @@ export const ToolConfirmationMessage: React.FC< const settings = useSettings(); const allowPermanentApproval = settings.merged.security.enablePermanentToolApproval && - !config.getDisableAlwaysAllow(); + (config?.getDisableAlwaysAllow ? !config.getDisableAlwaysAllow() : true); const handlesOwnUI = confirmationDetails.type === 'ask_user' || confirmationDetails.type === 'exit_plan_mode'; const isTrustedFolder = - config.isTrustedFolder() && !config.getDisableAlwaysAllow(); + config?.isTrustedFolder?.() && + (config?.getDisableAlwaysAllow ? !config.getDisableAlwaysAllow() : true); const handleConfirm = useCallback( (outcome: ToolConfirmationOutcome, payload?: ToolConfirmationPayload) => { diff --git a/packages/core/src/scheduler/policy.ts b/packages/core/src/scheduler/policy.ts index ca84447261..a59aa16510 100644 --- a/packages/core/src/scheduler/policy.ts +++ b/packages/core/src/scheduler/policy.ts @@ -119,7 +119,7 @@ export async function updatePolicy( ): Promise { // Mode Transitions (AUTO_EDIT) if (isAutoEditTransition(tool, outcome)) { - context.config.setApprovalMode(ApprovalMode.AUTO_EDIT); + context.config?.setApprovalMode?.(ApprovalMode.AUTO_EDIT); return; } @@ -128,9 +128,8 @@ export async function updatePolicy( if (outcome === ToolConfirmationOutcome.ProceedAlwaysAndSave) { // If folder is trusted and workspace policies are enabled, we prefer workspace scope. if ( - context.config && - context.config.isTrustedFolder() && - context.config.getWorkspacePoliciesDir() !== undefined + context.config?.isTrustedFolder?.() && + context.config?.getWorkspacePoliciesDir?.() !== undefined ) { persistScope = 'workspace'; } else { diff --git a/packages/core/src/tools/shell.ts b/packages/core/src/tools/shell.ts index 8917d281bd..88248b44ca 100644 --- a/packages/core/src/tools/shell.ts +++ b/packages/core/src/tools/shell.ts @@ -45,6 +45,7 @@ import type { MessageBus } from '../confirmation-bus/message-bus.js'; import { getShellDefinition } from './definitions/coreTools.js'; import { resolveToolDeclaration } from './definitions/resolver.js'; import type { AgentLoopContext } from '../config/agent-loop-context.js'; +import type { Config } from '../config/config.js'; export const OUTPUT_UPDATE_INTERVAL_MS = 1000; @@ -62,14 +63,23 @@ export class ShellToolInvocation extends BaseToolInvocation< ShellToolParams, ToolResult > { + private readonly config: Config; + private readonly geminiClient?: GeminiClient; + constructor( - private readonly context: AgentLoopContext, + context: Config | AgentLoopContext, params: ShellToolParams, messageBus: MessageBus, _toolName?: string, _toolDisplayName?: string, ) { super(params, messageBus, _toolName, _toolDisplayName); + if ('config' in context) { + this.config = context.config; + this.geminiClient = context.geminiClient; + } else { + this.config = context; + } } getDescription(): string { @@ -168,7 +178,7 @@ export class ShellToolInvocation extends BaseToolInvocation< .toString('hex')}.tmp`; const tempFilePath = path.join(os.tmpdir(), tempFileName); - const timeoutMs = this.context.config.getShellToolInactivityTimeout(); + const timeoutMs = this.config.getShellToolInactivityTimeout(); const timeoutController = new AbortController(); let timeoutTimer: NodeJS.Timeout | undefined; @@ -189,10 +199,10 @@ export class ShellToolInvocation extends BaseToolInvocation< })(); const cwd = this.params.dir_path - ? path.resolve(this.context.config.getTargetDir(), this.params.dir_path) - : this.context.config.getTargetDir(); + ? path.resolve(this.config.getTargetDir(), this.params.dir_path) + : this.config.getTargetDir(); - const validationError = this.context.config.validatePathAccess(cwd); + const validationError = this.config.validatePathAccess(cwd); if (validationError) { return { llmContent: validationError, @@ -271,14 +281,14 @@ export class ShellToolInvocation extends BaseToolInvocation< } }, combinedController.signal, - this.context.config.getEnableInteractiveShell(), + this.config.getEnableInteractiveShell(), { ...shellExecutionConfig, pager: 'cat', sanitizationConfig: shellExecutionConfig?.sanitizationConfig ?? - this.context.config.sanitizationConfig, - sandboxManager: this.context.config.sandboxManager, + this.config.sanitizationConfig, + sandboxManager: this.config.sandboxManager, }, ); @@ -383,7 +393,7 @@ export class ShellToolInvocation extends BaseToolInvocation< } let returnDisplayMessage = ''; - if (this.context.config.getDebugMode()) { + if (this.config.getDebugMode()) { returnDisplayMessage = llmContent; } else { if (this.params.is_background || result.backgrounded) { @@ -412,8 +422,7 @@ export class ShellToolInvocation extends BaseToolInvocation< } } - const summarizeConfig = - this.context.config.getSummarizeToolOutputConfig(); + const summarizeConfig = this.config.getSummarizeToolOutputConfig(); const executionError = result.error ? { error: { @@ -424,10 +433,10 @@ export class ShellToolInvocation extends BaseToolInvocation< : {}; if (summarizeConfig && summarizeConfig[SHELL_TOOL_NAME]) { const summary = await summarizeToolOutput( - this.context.config, + this.config, { model: 'summarizer-shell' }, llmContent, - this.context.geminiClient, + this.geminiClient ?? this.config.getGeminiClient(), signal, ); return { @@ -461,30 +470,38 @@ export class ShellTool extends BaseDeclarativeTool< ToolResult > { static readonly Name = SHELL_TOOL_NAME; + private readonly config: Config; + private readonly geminiClient?: GeminiClient; constructor( - private readonly context: AgentLoopContext, + context: Config | AgentLoopContext, messageBus: MessageBus, ) { void initializeShellParsers().catch(() => { // Errors are surfaced when parsing commands. }); + const config = 'config' in context ? context.config : context; const definition = getShellDefinition( - context.config.getEnableInteractiveShell(), - context.config.getEnableShellOutputEfficiency(), + config.getEnableInteractiveShell(), + config.getEnableShellOutputEfficiency(), ); super( ShellTool.Name, 'Shell', definition.base.description!, - Kind.Execute, + Kind.Shell, definition.base.parametersJsonSchema, messageBus, - false, // output is not markdown - true, // output can be updated + false, // isOutputMarkdown + true, // canUpdateOutput ); + this.config = config; + if ('config' in context) { + this.geminiClient = context.geminiClient; + } } + protected override validateToolParamValues( params: ShellToolParams, ): string | null { @@ -494,10 +511,10 @@ export class ShellTool extends BaseDeclarativeTool< if (params.dir_path) { const resolvedPath = path.resolve( - this.context.config.getTargetDir(), + this.config.getTargetDir(), params.dir_path, ); - return this.context.config.validatePathAccess(resolvedPath); + return this.config.validatePathAccess(resolvedPath); } return null; } @@ -509,7 +526,7 @@ export class ShellTool extends BaseDeclarativeTool< _toolDisplayName?: string, ): ToolInvocation { return new ShellToolInvocation( - this.context.config, + { config: this.config, geminiClient: this.geminiClient } as any, params, messageBus, _toolName, @@ -519,8 +536,8 @@ export class ShellTool extends BaseDeclarativeTool< override getSchema(modelId?: string) { const definition = getShellDefinition( - this.context.config.getEnableInteractiveShell(), - this.context.config.getEnableShellOutputEfficiency(), + this.config.getEnableInteractiveShell(), + this.config.getEnableShellOutputEfficiency(), ); return resolveToolDeclaration(definition, modelId); } diff --git a/packages/core/src/tools/web-fetch.ts b/packages/core/src/tools/web-fetch.ts index 27a60c4259..a37b7803ba 100644 --- a/packages/core/src/tools/web-fetch.ts +++ b/packages/core/src/tools/web-fetch.ts @@ -38,6 +38,8 @@ import { WEB_FETCH_DEFINITION } from './definitions/coreTools.js'; import { resolveToolDeclaration } from './definitions/resolver.js'; import { LRUCache } from 'mnemonist'; import type { AgentLoopContext } from '../config/agent-loop-context.js'; +import type { Config } from '../config/config.js'; +import type { GeminiClient } from '../core/client.js'; const URL_FETCH_TIMEOUT_MS = 10000; const MAX_CONTENT_LENGTH = 250000; @@ -224,18 +226,27 @@ class WebFetchToolInvocation extends BaseToolInvocation< WebFetchToolParams, ToolResult > { + private readonly config: Config; + private readonly geminiClient?: GeminiClient; + constructor( - private readonly context: AgentLoopContext, + context: Config | AgentLoopContext, params: WebFetchToolParams, messageBus: MessageBus, _toolName?: string, _toolDisplayName?: string, ) { super(params, messageBus, _toolName, _toolDisplayName); + if ('config' in context) { + this.config = context.config; + this.geminiClient = context.geminiClient; + } else { + this.config = context; + } } private handleRetry(attempt: number, error: unknown, delayMs: number): void { - const maxAttempts = this.context.config.getMaxAttempts(); + const maxAttempts = this.config.getMaxAttempts(); const modelName = 'Web Fetch'; const errorType = getRetryErrorType(error); @@ -248,7 +259,7 @@ class WebFetchToolInvocation extends BaseToolInvocation< }); logNetworkRetryAttempt( - this.context.config, + this.config, new NetworkRetryAttemptEvent( attempt, maxAttempts, @@ -302,13 +313,12 @@ class WebFetchToolInvocation extends BaseToolInvocation< return res; }, { - retryFetchErrors: this.context.config.getRetryFetchErrors(), + retryFetchErrors: this.config.getRetryFetchErrors(), onRetry: (attempt, error, delayMs) => this.handleRetry(attempt, error, delayMs), signal, }, - ); - + ); const bodyBuffer = await this.readResponseWithLimit( response, MAX_EXPERIMENTAL_FETCH_SIZE, @@ -350,7 +360,7 @@ class WebFetchToolInvocation extends BaseToolInvocation< `[WebFetchTool] Skipped private or local host: ${url}`, ); logWebFetchFallbackAttempt( - this.context.config, + this.config, new WebFetchFallbackAttemptEvent('private_ip_skipped'), ); skipped.push(`[Blocked Host] ${url}`); @@ -435,7 +445,7 @@ class WebFetchToolInvocation extends BaseToolInvocation< .join('\n'); try { - const geminiClient = this.context.geminiClient; + const geminiClient = this.geminiClient ?? this.config.getGeminiClient(); const fallbackPrompt = `Follow the user's instructions below using the provided webpage content. @@ -518,7 +528,7 @@ ${aggregatedContent} ): Promise { // Check for AUTO_EDIT approval mode. This tool has a specific behavior // where ProceedAlways switches the entire session to AUTO_EDIT. - if (this.context.config.getApprovalMode() === ApprovalMode.AUTO_EDIT) { + if (this.config.getApprovalMode() === ApprovalMode.AUTO_EDIT) { return false; } @@ -641,7 +651,7 @@ ${aggregatedContent} return res; }, { - retryFetchErrors: this.context.config.getRetryFetchErrors(), + retryFetchErrors: this.config.getRetryFetchErrors(), onRetry: (attempt, error, delayMs) => this.handleRetry(attempt, error, delayMs), signal, @@ -752,7 +762,7 @@ Response: ${truncateString(rawResponseText, 10000, '\n\n... [Error response trun } async execute(signal: AbortSignal): Promise { - if (this.context.config.getDirectWebFetch()) { + if (this.config.getDirectWebFetch()) { return this.executeExperimental(signal); } const userPrompt = this.params.prompt!; @@ -775,7 +785,7 @@ Response: ${truncateString(rawResponseText, 10000, '\n\n... [Error response trun } try { - const geminiClient = this.context.geminiClient; + const geminiClient = this.geminiClient ?? this.config.getGeminiClient(); const sanitizedPrompt = `Follow the user's instructions to process the authorized URLs. @@ -867,7 +877,7 @@ ${toFetch.join('\n')} `[WebFetchTool] Primary fetch failed, falling back: ${getErrorMessage(error)}`, ); logWebFetchFallbackAttempt( - this.context.config, + this.config, new WebFetchFallbackAttemptEvent('primary_failed'), ); // Simple All-or-Nothing Fallback @@ -884,11 +894,14 @@ export class WebFetchTool extends BaseDeclarativeTool< ToolResult > { static readonly Name = WEB_FETCH_TOOL_NAME; + private readonly config: Config; + private readonly geminiClient?: GeminiClient; constructor( - private readonly context: AgentLoopContext, + context: Config | AgentLoopContext, messageBus: MessageBus, ) { + const config = 'config' in context ? context.config : context; super( WebFetchTool.Name, 'WebFetch', @@ -899,12 +912,16 @@ export class WebFetchTool extends BaseDeclarativeTool< true, // isOutputMarkdown false, // canUpdateOutput ); + this.config = config; + if ('config' in context) { + this.geminiClient = context.geminiClient; + } } protected override validateToolParamValues( params: WebFetchToolParams, ): string | null { - if (this.context.config.getDirectWebFetch()) { + if (this.config.getDirectWebFetch()) { if (!params.url) { return "The 'url' parameter is required."; } @@ -940,7 +957,7 @@ export class WebFetchTool extends BaseDeclarativeTool< _toolDisplayName?: string, ): ToolInvocation { return new WebFetchToolInvocation( - this.context, + { config: this.config, geminiClient: this.geminiClient } as any, params, messageBus, _toolName, @@ -950,7 +967,7 @@ export class WebFetchTool extends BaseDeclarativeTool< override getSchema(modelId?: string) { const schema = resolveToolDeclaration(WEB_FETCH_DEFINITION, modelId); - if (this.context.config.getDirectWebFetch()) { + if (this.config.getDirectWebFetch()) { return { ...schema, description: diff --git a/packages/core/src/tools/web-search.ts b/packages/core/src/tools/web-search.ts index 18132d2c35..95baf3ff23 100644 --- a/packages/core/src/tools/web-search.ts +++ b/packages/core/src/tools/web-search.ts @@ -23,6 +23,8 @@ import { WEB_SEARCH_DEFINITION } from './definitions/coreTools.js'; import { resolveToolDeclaration } from './definitions/resolver.js'; import { LlmRole } from '../telemetry/llmRole.js'; import type { AgentLoopContext } from '../config/agent-loop-context.js'; +import type { Config } from '../config/config.js'; +import type { GeminiClient } from '../core/client.js'; interface GroundingChunkWeb { uri?: string; @@ -70,14 +72,23 @@ class WebSearchToolInvocation extends BaseToolInvocation< WebSearchToolParams, WebSearchToolResult > { + private readonly config: Config; + private readonly geminiClient?: GeminiClient; + constructor( - private readonly context: AgentLoopContext, + context: Config | AgentLoopContext, params: WebSearchToolParams, messageBus: MessageBus, _toolName?: string, _toolDisplayName?: string, ) { super(params, messageBus, _toolName, _toolDisplayName); + if ('config' in context) { + this.config = context.config; + this.geminiClient = context.geminiClient; + } else { + this.config = context; + } } override getDescription(): string { @@ -85,7 +96,7 @@ class WebSearchToolInvocation extends BaseToolInvocation< } async execute(signal: AbortSignal): Promise { - const geminiClient = this.context.geminiClient; + const geminiClient = this.geminiClient ?? this.config.getGeminiClient(); try { const response = await geminiClient.generateContent( @@ -205,11 +216,14 @@ export class WebSearchTool extends BaseDeclarativeTool< WebSearchToolResult > { static readonly Name = WEB_SEARCH_TOOL_NAME; + private readonly config: Config; + private readonly geminiClient?: GeminiClient; constructor( - private readonly context: AgentLoopContext, + context: Config | AgentLoopContext, messageBus: MessageBus, ) { + const config = 'config' in context ? context.config : context; super( WebSearchTool.Name, 'GoogleSearch', @@ -220,6 +234,10 @@ export class WebSearchTool extends BaseDeclarativeTool< true, // isOutputMarkdown false, // canUpdateOutput ); + this.config = config; + if ('config' in context) { + this.geminiClient = context.geminiClient; + } } /** @@ -243,7 +261,7 @@ export class WebSearchTool extends BaseDeclarativeTool< _toolDisplayName?: string, ): ToolInvocation { return new WebSearchToolInvocation( - this.context.config, + { config: this.config, geminiClient: this.geminiClient } as any, params, messageBus ?? this.messageBus, _toolName, diff --git a/plans/implemented/fix-trusted-folder-error.md b/plans/implemented/fix-trusted-folder-error.md new file mode 100644 index 0000000000..457a5bc258 --- /dev/null +++ b/plans/implemented/fix-trusted-folder-error.md @@ -0,0 +1,67 @@ +# Implementation Plan - Fix Context Initialization Mismatch in Core Tools + +The Gemini CLI is experiencing various "Cannot read properties of undefined" errors (notably `isTrustedFolder` and `publish`) during tool execution and confirmation. This is caused by a structural mismatch where the global `Config` object is passed to tool constructors that now expect an `AgentLoopContext`. + +## Background & Reproducibility Analysis + +### Root Cause Analysis (Regression) +The regression was introduced in commit **`de656f01d7`** (PR **#22115**), titled *"feat(core): Fully migrate packages/core to AgentLoopContext"*. + +In this PR: +- Tool constructors (e.g., `ShellTool`, `WebFetchTool`, `WebSearchTool`) were updated to expect an `AgentLoopContext` instead of a `Config` object. +- `AgentLoopContext` is an interface that includes a `.config` property. +- However, in `packages/core/src/config/config.ts`, the global `Config` object still instantiates these tools by passing `this` (the `Config` instance itself). +- While `Config` implements some getters that overlap with `AgentLoopContext` (like `geminiClient`), it does **not** have a `.config` property. +- Consequently, internal tool calls to `this.context.config.someMethod()` fail because `this.context.config` is undefined. +- Additionally, some parts of the system expect a fully initialized `messageBus` on the context, which can lead to "Cannot read properties of undefined (reading 'publish')" if the context isn't correctly structured or if there's a race in initialization. + +### Why this might not be reproducible for all users: +- **Sub-agent Usage:** Sub-agents correctly initialize tools with a full `AgentLoopContext` via `LocalAgentExecutor`. +- **Tool Selection:** The bug only surfaces in tools that specifically access `context.config` or certain context-scoped services. +- **Initialization Order:** Depending on how the CLI is invoked (interactive vs. non-interactive), the `Config` object might be in different states of initialization. + +## Objective +Standardize tool initialization so that built-in tools can correctly resolve their required configuration and services whether they are running in the main loop (initialized with `Config`) or as a sub-agent (initialized with `AgentLoopContext`). + +## Key Files & Context +- `packages/core/src/config/config.ts`: Where built-in tools are incorrectly instantiated with `this`. +- `packages/core/src/tools/shell.ts`: `ShellTool` constructor expects `AgentLoopContext`. +- `packages/core/src/tools/web-fetch.ts`: `WebFetchTool` constructor expects `AgentLoopContext`. +- `packages/core/src/tools/web-search.ts`: `WebSearchTool` constructor expects `AgentLoopContext`. +- `packages/core/src/scheduler/policy.ts`: Accesses `context.config.isTrustedFolder()`. +- `packages/core/src/scheduler/scheduler.ts`: Initializes its own `messageBus` and `context`. + +## Implementation Steps + +### Phase 0: Preparation +1. **Update Plan:** (This file) Broaden the scope from just "isTrusted" to "Context Mismatch". +2. **GitHub Issue:** Update GitHub issue #23174 with the refined root cause analysis. + +### Phase 1: Robust Tool Context Handling +The built-in tools need to handle being initialized by either the global `Config` or an `AgentLoopContext`. + +- **Update `packages/core/src/tools/shell.ts`**, **`web-fetch.ts`**, and **`web-search.ts`**: + - Update constructors to accept `Config | AgentLoopContext`. + - Internalize logic to safely resolve `config`, `geminiClient`, and other services. + - Example: + ```typescript + this.config = 'config' in context ? context.config : context; + ``` + +### Phase 2: UI and Policy Hardening +- **Update `packages/cli/src/ui/components/messages/ToolConfirmationMessage.tsx`**: Add safety checks for `config`. +- **Update `packages/core/src/scheduler/policy.ts`**: Ensure `context.config` exists before use. + +### Phase 3: Scheduler Verification +- Ensure `Scheduler` and `SchedulerStateManager` are receiving a correctly structured `messageBus`. + +## Verification & Testing + +### Automated Tests +- Create unit tests that specifically instantiate tools with a raw `Config` object and call methods that access `this.config`. +- Verify `npm run test:core` passes. + +### Manual Verification +1. Trigger shell commands in the interactive CLI. +2. Attempt "Allow all" actions. +3. Verify no "undefined" crashes occur.