From c06794b3c671c3c51ee705456b08ed6a18166228 Mon Sep 17 00:00:00 2001 From: Sri Pasumarthi <111310667+sripasg@users.noreply.github.com> Date: Wed, 25 Mar 2026 09:52:21 -0700 Subject: [PATCH] fix(acp): allow attachments by adding a permission prompt (#23680) --- packages/cli/src/acp/acpClient.test.ts | 183 +++++++++++++++++- packages/cli/src/acp/acpClient.ts | 250 +++++++++++++++++++++++-- 2 files changed, 412 insertions(+), 21 deletions(-) diff --git a/packages/cli/src/acp/acpClient.test.ts b/packages/cli/src/acp/acpClient.test.ts index 3ae71e6ebb..e10f0e3e3d 100644 --- a/packages/cli/src/acp/acpClient.test.ts +++ b/packages/cli/src/acp/acpClient.test.ts @@ -21,13 +21,13 @@ import { AuthType, ToolConfirmationOutcome, StreamEventType, - isWithinRoot, ReadManyFilesTool, type GeminiChat, type Config, type MessageBus, LlmRole, type GitService, + processSingleFileContent, } from '@google/gemini-cli-core'; import { SettingScope, @@ -111,7 +111,6 @@ vi.mock( }), })), logToolCall: vi.fn(), - isWithinRoot: vi.fn().mockReturnValue(true), LlmRole: { MAIN: 'main', SUBAGENT: 'subagent', @@ -134,6 +133,7 @@ vi.mock( Cancelled: 'cancelled', AwaitingApproval: 'awaiting_approval', }, + processSingleFileContent: vi.fn(), }; }, ); @@ -177,6 +177,10 @@ describe('GeminiAgent', () => { getHasAccessToPreviewModel: vi.fn().mockReturnValue(false), getCheckpointingEnabled: vi.fn().mockReturnValue(false), getDisableAlwaysAllow: vi.fn().mockReturnValue(false), + validatePathAccess: vi.fn().mockReturnValue(null), + getWorkspaceContext: vi.fn().mockReturnValue({ + addReadOnlyPath: vi.fn(), + }), get config() { return this; }, @@ -191,6 +195,7 @@ describe('GeminiAgent', () => { mockArgv = {} as unknown as CliArgs; mockConnection = { sessionUpdate: vi.fn(), + requestPermission: vi.fn(), } as unknown as Mocked; (loadCliConfig as unknown as Mock).mockResolvedValue(mockConfig); @@ -648,6 +653,7 @@ describe('Session', () => { shouldIgnoreFile: vi.fn().mockReturnValue(false), }), getFileFilteringOptions: vi.fn().mockReturnValue({}), + getFileSystemService: vi.fn().mockReturnValue({}), getTargetDir: vi.fn().mockReturnValue('/tmp'), getEnableRecursiveFileSearch: vi.fn().mockReturnValue(false), getDebugMode: vi.fn().mockReturnValue(false), @@ -657,6 +663,10 @@ describe('Session', () => { isPlanEnabled: vi.fn().mockReturnValue(true), getCheckpointingEnabled: vi.fn().mockReturnValue(false), getGitService: vi.fn().mockResolvedValue({} as GitService), + validatePathAccess: vi.fn().mockReturnValue(null), + getWorkspaceContext: vi.fn().mockReturnValue({ + addReadOnlyPath: vi.fn(), + }), waitForMcpInit: vi.fn(), getDisableAlwaysAllow: vi.fn().mockReturnValue(false), get config() { @@ -1356,7 +1366,6 @@ describe('Session', () => { (fs.stat as unknown as Mock).mockResolvedValue({ isDirectory: () => false, }); - (isWithinRoot as unknown as Mock).mockReturnValue(true); const stream = createMockStream([ { @@ -1414,7 +1423,6 @@ describe('Session', () => { (fs.stat as unknown as Mock).mockResolvedValue({ isDirectory: () => false, }); - (isWithinRoot as unknown as Mock).mockReturnValue(true); const MockReadManyFilesTool = ReadManyFilesTool as unknown as Mock; MockReadManyFilesTool.mockImplementationOnce(() => ({ @@ -1468,6 +1476,172 @@ describe('Session', () => { ); }); + it('should handle @path validation error and bubble it to user', async () => { + mockConfig.getTargetDir.mockReturnValue('/workspace'); + (path.resolve as unknown as Mock).mockReturnValue('/tmp/disallowed.txt'); + mockConfig.validatePathAccess.mockReturnValue('Path is outside workspace'); + + // Force fs.stat to fail to skip direct reading and triggers the warning + (fs.stat as unknown as Mock).mockRejectedValue(new Error('File not found')); + + const stream = createMockStream([ + { + type: StreamEventType.CHUNK, + value: { candidates: [] }, + }, + ]); + mockChat.sendMessageStream.mockResolvedValue(stream); + + await session.prompt({ + sessionId: 'session-1', + prompt: [ + { + type: 'resource_link', + uri: 'file://disallowed.txt', + mimeType: 'text/plain', + name: 'disallowed.txt', + }, + ], + }); + + // Verify warning sent via sendUpdate + expect(mockConnection.sessionUpdate).toHaveBeenCalledWith( + expect.objectContaining({ + update: expect.objectContaining({ + sessionUpdate: 'agent_thought_chunk', + content: expect.objectContaining({ + text: expect.stringContaining( + 'Warning: skipping access to `disallowed.txt`. Reason: Path is outside workspace', + ), + }), + }), + }), + ); + }); + + it('should read absolute file directly if outside workspace', async () => { + mockConfig.getTargetDir.mockReturnValue('/workspace'); + const testFilePath = '/tmp/custom.txt'; + (path.resolve as unknown as Mock).mockReturnValue(testFilePath); + mockConfig.validatePathAccess.mockReturnValue('Path is outside workspace'); + + mockConnection.requestPermission.mockResolvedValue({ + outcome: { + outcome: 'selected', + optionId: ToolConfirmationOutcome.ProceedOnce, + }, + } as unknown as acp.RequestPermissionResponse); + + const mockStats = { + isFile: () => true, + isDirectory: () => false, + }; + (fs.stat as unknown as Mock).mockResolvedValue(mockStats); + (processSingleFileContent as unknown as Mock).mockResolvedValue({ + llmContent: 'Absolute File Content', + }); + + const stream = createMockStream([ + { + type: StreamEventType.CHUNK, + value: { candidates: [] }, + }, + ]); + mockChat.sendMessageStream.mockResolvedValue(stream); + + await session.prompt({ + sessionId: 'session-1', + prompt: [ + { + type: 'resource_link', + uri: `file://${testFilePath}`, + mimeType: 'text/plain', + name: 'custom.txt', + }, + ], + }); + + expect(processSingleFileContent).toHaveBeenCalledWith( + testFilePath, + expect.anything(), + expect.anything(), + ); + + // Verify content appended to sendMessageStream parts + expect(mockChat.sendMessageStream).toHaveBeenCalledWith( + expect.anything(), + expect.arrayContaining([ + expect.objectContaining({ + text: 'Absolute File Content', + }), + ]), + expect.anything(), + expect.any(AbortSignal), + expect.anything(), + ); + }); + + it('should read escaping relative file directly if outside workspace', async () => { + mockConfig.getTargetDir.mockReturnValue('/workspace'); + const testFilePath = '../../custom.txt'; + (path.resolve as unknown as Mock).mockReturnValue('/custom.txt'); + mockConfig.validatePathAccess.mockReturnValue('Path is outside workspace'); + + mockConnection.requestPermission.mockResolvedValue({ + outcome: { + outcome: 'selected', + optionId: ToolConfirmationOutcome.ProceedOnce, + }, + } as unknown as acp.RequestPermissionResponse); + + const mockStats = { + isFile: () => true, + isDirectory: () => false, + }; + (fs.stat as unknown as Mock).mockResolvedValue(mockStats); + (processSingleFileContent as unknown as Mock).mockResolvedValue({ + llmContent: 'Escaping Relative File Content', + }); + + const stream = createMockStream([ + { + type: StreamEventType.CHUNK, + value: { candidates: [] }, + }, + ]); + mockChat.sendMessageStream.mockResolvedValue(stream); + + await session.prompt({ + sessionId: 'session-1', + prompt: [ + { + type: 'resource_link', + uri: `file://${testFilePath}`, + mimeType: 'text/plain', + name: 'custom.txt', + }, + ], + }); + + expect(processSingleFileContent).toHaveBeenCalledWith( + '/custom.txt', + expect.any(String), + expect.anything(), + ); + + expect(mockChat.sendMessageStream).toHaveBeenCalledWith( + expect.anything(), + expect.arrayContaining([ + expect.objectContaining({ + text: 'Escaping Relative File Content', + }), + ]), + expect.anything(), + expect.any(AbortSignal), + expect.anything(), + ); + }); + it('should handle cancellation during prompt', async () => { let streamController: ReadableStreamDefaultController; const stream = new ReadableStream({ @@ -1666,7 +1840,6 @@ describe('Session', () => { (fs.stat as unknown as Mock).mockResolvedValue({ isDirectory: () => true, }); - (isWithinRoot as unknown as Mock).mockReturnValue(true); const stream = createMockStream([ { diff --git a/packages/cli/src/acp/acpClient.ts b/packages/cli/src/acp/acpClient.ts index 57903822e9..1a300413b0 100644 --- a/packages/cli/src/acp/acpClient.ts +++ b/packages/cli/src/acp/acpClient.ts @@ -47,6 +47,7 @@ import { DEFAULT_GEMINI_MODEL_AUTO, PREVIEW_GEMINI_MODEL_AUTO, getDisplayString, + processSingleFileContent, type AgentLoopContext, } from '@google/gemini-cli-core'; import * as acp from '@agentclientprotocol/sdk'; @@ -73,6 +74,17 @@ import { runExitCleanup } from '../utils/cleanup.js'; import { SessionSelector } from '../utils/sessionUtils.js'; import { CommandHandler } from './commandHandler.js'; + +const RequestPermissionResponseSchema = z.object({ + outcome: z.discriminatedUnion('outcome', [ + z.object({ outcome: z.literal('cancelled') }), + z.object({ + outcome: z.literal('selected'), + optionId: z.string(), + }), + ]), +}); + export async function runAcpClient( config: Config, settings: LoadedSettings, @@ -1011,10 +1023,12 @@ export class Session { }, }; - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - const output = await this.connection.requestPermission(params); + const output = RequestPermissionResponseSchema.parse( + await this.connection.requestPermission(params), + ); + const outcome = - output.outcome.outcome === CoreToolCallStatus.Cancelled + output.outcome.outcome === 'cancelled' ? ToolConfirmationOutcome.Cancel : z .nativeEnum(ToolConfirmationOutcome) @@ -1225,6 +1239,11 @@ export class Session { const pathSpecsToRead: string[] = []; const contentLabelsForDisplay: string[] = []; const ignoredPaths: string[] = []; + const directContents: Array<{ + spec: string; + content?: string; + part?: Part; + }> = []; const toolRegistry = this.context.toolRegistry; const readManyFilesTool = new ReadManyFilesTool( @@ -1247,28 +1266,197 @@ export class Session { } let currentPathSpec = pathName; let resolvedSuccessfully = false; + let readDirectly = false; try { const absolutePath = path.resolve( this.context.config.getTargetDir(), pathName, ); - if (isWithinRoot(absolutePath, this.context.config.getTargetDir())) { - const stats = await fs.stat(absolutePath); - if (stats.isDirectory()) { - currentPathSpec = pathName.endsWith('/') - ? `${pathName}**` - : `${pathName}/**`; + + let validationError = this.context.config.validatePathAccess( + absolutePath, + 'read', + ); + + // We ask the user for explicit permission to read them if outside sandboxed workspace boundaries (and not already authorized). + if ( + validationError && + !isWithinRoot(absolutePath, this.context.config.getTargetDir()) + ) { + try { + const stats = await fs.stat(absolutePath); + if (stats.isFile()) { + const syntheticCallId = `resolve-prompt-${pathName}-${randomUUID()}`; + const params = { + sessionId: this.id, + options: [ + { + optionId: ToolConfirmationOutcome.ProceedOnce, + name: 'Allow once', + kind: 'allow_once', + }, + { + optionId: ToolConfirmationOutcome.Cancel, + name: 'Deny', + kind: 'reject_once', + }, + ] as acp.PermissionOption[], + toolCall: { + toolCallId: syntheticCallId, + status: 'pending', + title: `Allow access to absolute path: ${pathName}`, + content: [ + { + type: 'content', + content: { + type: 'text', + text: `The Agent needs access to read an attached file outside your workspace: ${pathName}`, + }, + }, + ], + locations: [], + kind: 'read', + }, + }; + + const output = RequestPermissionResponseSchema.parse( + await this.connection.requestPermission(params), + ); + + const outcome = + output.outcome.outcome === 'cancelled' + ? ToolConfirmationOutcome.Cancel + : z + .nativeEnum(ToolConfirmationOutcome) + .parse(output.outcome.optionId); + + if (outcome === ToolConfirmationOutcome.ProceedOnce) { + this.context.config + .getWorkspaceContext() + .addReadOnlyPath(absolutePath); + validationError = null; + } else { + this.debug( + `Direct read authorization denied for absolute path ${pathName}`, + ); + directContents.push({ + spec: pathName, + content: `[Warning: Access to absolute path \`${pathName}\` denied by user.]`, + }); + continue; + } + } + } catch (error) { this.debug( - `Path ${pathName} resolved to directory, using glob: ${currentPathSpec}`, + `Failed to request permission for absolute attachment ${pathName}: ${getErrorMessage(error)}`, ); - } else { - this.debug(`Path ${pathName} resolved to file: ${currentPathSpec}`); + await this.sendUpdate({ + sessionUpdate: 'agent_thought_chunk', + content: { + type: 'text', + text: `Warning: Failed to display permission dialog for \`${absolutePath}\`. Error: ${getErrorMessage(error)}`, + }, + }); + } + } + + if (!validationError) { + // If it's an absolute path that is authorized (e.g. added via readOnlyPaths), + // read it directly to avoid ReadManyFilesTool absolute path resolution issues. + if ( + (path.isAbsolute(pathName) || + !isWithinRoot( + absolutePath, + this.context.config.getTargetDir(), + )) && + !readDirectly + ) { + try { + const stats = await fs.stat(absolutePath); + if (stats.isFile()) { + const fileReadResult = await processSingleFileContent( + absolutePath, + this.context.config.getTargetDir(), + this.context.config.getFileSystemService(), + ); + + if (!fileReadResult.error) { + if ( + typeof fileReadResult.llmContent === 'object' && + 'inlineData' in fileReadResult.llmContent + ) { + directContents.push({ + spec: pathName, + part: fileReadResult.llmContent, + }); + } else if (typeof fileReadResult.llmContent === 'string') { + let contentToPush = fileReadResult.llmContent; + if (fileReadResult.isTruncated) { + contentToPush = `[WARNING: This file was truncated]\n\n${contentToPush}`; + } + directContents.push({ + spec: pathName, + content: contentToPush, + }); + } + readDirectly = true; + resolvedSuccessfully = true; + } else { + this.debug( + `Direct read failed for absolute path ${pathName}: ${fileReadResult.error}`, + ); + await this.sendUpdate({ + sessionUpdate: 'agent_thought_chunk', + content: { + type: 'text', + text: `Warning: file read failed for \`${pathName}\`. Reason: ${fileReadResult.error}`, + }, + }); + continue; + } + } + } catch (error) { + this.debug( + `File stat/access error for absolute path ${pathName}: ${getErrorMessage(error)}`, + ); + await this.sendUpdate({ + sessionUpdate: 'agent_thought_chunk', + content: { + type: 'text', + text: `Warning: file access failed for \`${pathName}\`. Reason: ${getErrorMessage(error)}`, + }, + }); + continue; + } + } + + if (!readDirectly) { + const stats = await fs.stat(absolutePath); + if (stats.isDirectory()) { + currentPathSpec = pathName.endsWith('/') + ? `${pathName}**` + : `${pathName}/**`; + this.debug( + `Path ${pathName} resolved to directory, using glob: ${currentPathSpec}`, + ); + } else { + this.debug( + `Path ${pathName} resolved to file: ${currentPathSpec}`, + ); + } + resolvedSuccessfully = true; } - resolvedSuccessfully = true; } else { this.debug( - `Path ${pathName} is outside the project directory. Skipping.`, + `Path ${pathName} access disallowed: ${validationError}. Skipping.`, ); + await this.sendUpdate({ + sessionUpdate: 'agent_thought_chunk', + content: { + type: 'text', + text: `Warning: skipping access to \`${pathName}\`. Reason: ${validationError}`, + }, + }); } } catch (error) { if (isNodeError(error) && error.code === 'ENOENT') { @@ -1328,7 +1516,9 @@ export class Session { } } if (resolvedSuccessfully) { - pathSpecsToRead.push(currentPathSpec); + if (!readDirectly) { + pathSpecsToRead.push(currentPathSpec); + } atPathToResolvedSpecMap.set(pathName, currentPathSpec); contentLabelsForDisplay.push(pathName); } @@ -1389,7 +1579,11 @@ export class Session { const processedQueryParts: Part[] = [{ text: initialQueryText }]; - if (pathSpecsToRead.length === 0 && embeddedContext.length === 0) { + if ( + pathSpecsToRead.length === 0 && + embeddedContext.length === 0 && + directContents.length === 0 + ) { // Fallback for lone "@" or completely invalid @-commands resulting in empty initialQueryText debugLogger.warn('No valid file paths found in @ commands to read.'); return [{ text: initialQueryText }]; @@ -1481,6 +1675,30 @@ export class Session { } } + if (directContents.length > 0) { + const hasReferenceStart = processedQueryParts.some( + (p) => + 'text' in p && + typeof p.text === 'string' && + p.text.includes(REFERENCE_CONTENT_START), + ); + if (!hasReferenceStart) { + processedQueryParts.push({ + text: `\n${REFERENCE_CONTENT_START}`, + }); + } + for (const item of directContents) { + processedQueryParts.push({ + text: `\nContent from @${item.spec}:\n`, + }); + if (item.content) { + processedQueryParts.push({ text: item.content }); + } else if (item.part) { + processedQueryParts.push(item.part); + } + } + } + if (embeddedContext.length > 0) { processedQueryParts.push({ text: '\n--- Content from referenced context ---',