diff --git a/core/src/common.ts b/core/src/common.ts index 23f628165..14c59eba4 100644 --- a/core/src/common.ts +++ b/core/src/common.ts @@ -302,6 +302,7 @@ export type {SkillRegistry} from './skills/skill_registry.js'; export {ListSkillsTool} from './tools/skill/list_skills_tool.js'; export {LoadSkillResourceTool} from './tools/skill/load_skill_resource_tool.js'; export {LoadSkillTool} from './tools/skill/load_skill_tool.js'; +export type {SkillScriptResult} from './tools/skill/script_output_utils.js'; export {SearchSkillsTool} from './tools/skill/search_skills_tool.js'; export {SkillToolset} from './tools/skill/skill_toolset.js'; diff --git a/core/src/tools/skill/run_skill_inline_script_tool.ts b/core/src/tools/skill/run_skill_inline_script_tool.ts index 1c0ad2ea2..b9cb196a2 100644 --- a/core/src/tools/skill/run_skill_inline_script_tool.ts +++ b/core/src/tools/skill/run_skill_inline_script_tool.ts @@ -9,8 +9,8 @@ import {Context} from '../../agents/context.js'; import {isLlmAgent} from '../../agents/llm_agent.js'; import {CodeExecutionLanguage} from '../../code_executors/code_execution_utils.js'; import {experimental} from '../../utils/experimental.js'; -import {materializeFiles} from '../../utils/file_utils.js'; import {BaseTool, RunAsyncToolRequest} from '../base_tool.js'; +import {materializeScriptOutputs} from './script_output_utils.js'; import {SkillToolset} from './skill_toolset.js'; /** @@ -140,10 +140,7 @@ export class RunSkillInlineScriptTool extends BaseTool { }, }); - // Final filename could be different if there was a collision, so update the result. - result.outputFiles = await materializeFiles(result.outputFiles); - - return result; + return materializeScriptOutputs(result, this.toolset.outputDir); } catch (e: unknown) { return { error: `Failed to execute inline script: ${(e as Error).message}`, diff --git a/core/src/tools/skill/run_skill_script_tool.ts b/core/src/tools/skill/run_skill_script_tool.ts index 87e7b7a8d..f1ea10733 100644 --- a/core/src/tools/skill/run_skill_script_tool.ts +++ b/core/src/tools/skill/run_skill_script_tool.ts @@ -17,8 +17,8 @@ import { getMimeTypeAndEncoding, getScriptLanguageByExtension, } from '../../utils/file_extension_utils.js'; -import {materializeFiles} from '../../utils/file_utils.js'; import {BaseTool, RunAsyncToolRequest} from '../base_tool.js'; +import {materializeScriptOutputs} from './script_output_utils.js'; import {SkillToolset} from './skill_toolset.js'; @experimental @@ -141,10 +141,7 @@ export class RunSkillScriptTool extends BaseTool { }, }); - // Final filename could be different if there was a collision, so update the result. - result.outputFiles = await materializeFiles(result.outputFiles); - - return result; + return materializeScriptOutputs(result, this.toolset.outputDir); } catch (e: unknown) { return { error: `Failed to execute script '${scriptPath}': ${(e as Error).message}`, diff --git a/core/src/tools/skill/script_output_utils.ts b/core/src/tools/skill/script_output_utils.ts new file mode 100644 index 000000000..bf6b93fff --- /dev/null +++ b/core/src/tools/skill/script_output_utils.ts @@ -0,0 +1,53 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import * as fs from 'node:fs/promises'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import {CodeExecutionResult} from '../../code_executors/code_execution_utils.js'; +import {materializeFiles} from '../../utils/file_utils.js'; + +/** + * The result of a skill script execution, annotated with the directory its + * output files were written to. + */ +export interface SkillScriptResult extends CodeExecutionResult { + /** + * Absolute path of the directory the output files were written to. Absent + * when the script produced no output files. + */ + outputDir?: string; +} + +/** + * Writes the output files of a skill script execution under `outputDir`, or + * into a fresh per-execution directory in the OS temp directory when it is + * unset. Nothing is written when the script produced no output files. + * + * @param result The result returned by the code executor. + * @param outputDir Directory to write the output files into. See + * `SkillToolset`'s option of the same name for the lifetime policy. + * @returns The result with each output file name rewritten relative to the + * output directory, plus the absolute `outputDir` the files went to. + */ +export async function materializeScriptOutputs( + result: CodeExecutionResult, + outputDir?: string, +): Promise { + if (result.outputFiles.length === 0) { + return result; + } + + const dir = outputDir + ? path.resolve(outputDir) + : await fs.mkdtemp(path.join(os.tmpdir(), 'adk-skill-outputs-')); + + return { + ...result, + outputFiles: await materializeFiles(result.outputFiles, dir), + outputDir: dir, + }; +} diff --git a/core/src/tools/skill/skill_toolset.ts b/core/src/tools/skill/skill_toolset.ts index e631fc215..3064c24e7 100644 --- a/core/src/tools/skill/skill_toolset.ts +++ b/core/src/tools/skill/skill_toolset.ts @@ -47,6 +47,7 @@ export class SkillToolset extends BaseToolset { public registry?: SkillRegistry; private toolCache = new Map(); private fetchedSkillCache = new Map>(); + public readonly outputDir?: string; constructor( skills: Record | Skill[], @@ -63,6 +64,22 @@ export class SkillToolset extends BaseToolset { * confirmation. */ allowInlineScripts?: boolean; + /** + * Directory that output files produced by `run_skill_script` and + * `run_skill_inline_script` are written into. A relative path is + * resolved against the host process's working directory. + * + * When unset, each execution that produces output files gets a fresh + * directory under the OS temp directory and the tool response reports + * its absolute path. Set this to keep script output in a location the + * application manages. + * + * The directory is never deleted, unlike the code executor's own scratch + * directory: it holds the artifacts the script was asked to produce. + * Unconfigured runs therefore rely on OS temp-directory cleanup, so an + * application that needs a managed lifetime should set this. + */ + outputDir?: string; } = {}, ) { super([], 'adk_skill_toolset'); @@ -72,6 +89,7 @@ export class SkillToolset extends BaseToolset { this.codeExecutor = options.codeExecutor; this.additionalTools = options.additionalTools || []; this.registry = options.registry; + this.outputDir = options.outputDir; this.tools = [ new ListSkillsTool(this), diff --git a/core/src/utils/file_utils.ts b/core/src/utils/file_utils.ts index 54dc54a04..1b956a786 100644 --- a/core/src/utils/file_utils.ts +++ b/core/src/utils/file_utils.ts @@ -9,19 +9,44 @@ import * as path from 'node:path'; import {File} from '../code_executors/code_execution_utils.js'; /** - * Creates files with the given paths in the current working directory. - * @param files The files to materialize. + * Reports whether `fullPath` is a strict descendant of `baseDir`. Both + * arguments must already be resolved absolute paths. + */ +function isContained(baseDir: string, fullPath: string): boolean { + const rel = path.relative(baseDir, fullPath); + + return ( + rel !== '' && !path.isAbsolute(rel) && !rel.split(path.sep).includes('..') + ); +} + +/** + * Writes the given in-memory files under `dir`, creating parent directories as + * needed. An existing file is never overwritten: a numeric suffix is appended + * instead (`report.txt` -> `report_2.txt` -> `report_3.txt`). + * + * A name that does not resolve to a strict descendant of `dir` is rejected with + * a `Path traversal detected` error. That containment check is a lexical + * comparison of resolved paths and is **not** a sandbox: it does not survive + * symlinks, hardlinks, bind mounts, or a TOCTOU race between the check and the + * write. + * + * @param files The files to materialize. `name` is updated in place when a + * collision forces a rename. + * @param dir Base directory to write under. + * @returns The written files, each `name` rewritten to the final path relative + * to `dir`. */ export async function materializeFiles( files: File[], - dir = process.cwd(), + dir: string, ): Promise { const resolvedBaseDir = path.resolve(dir); const createdFiles: File[] = []; for (const file of files) { const fullPath = path.resolve(dir, file.name); - if (!fullPath.startsWith(resolvedBaseDir)) { + if (!isContained(resolvedBaseDir, fullPath)) { throw new Error( `Path traversal detected: ${file.name} resolves outside of ${dir}`, ); @@ -51,7 +76,7 @@ export async function materializeFiles( } } - if (!finalPath.startsWith(resolvedBaseDir)) { + if (!isContained(resolvedBaseDir, finalPath)) { throw new Error( `Path traversal detected: ${file.name} resolves outside of ${dir}`, ); diff --git a/core/test/tools/skills/run_skill_inline_script_tool_test.ts b/core/test/tools/skills/run_skill_inline_script_tool_test.ts index 36664b070..294db5cb5 100644 --- a/core/test/tools/skills/run_skill_inline_script_tool_test.ts +++ b/core/test/tools/skills/run_skill_inline_script_tool_test.ts @@ -16,14 +16,17 @@ import { LlmAgent, RunSkillInlineScriptErrorCode, RunSkillInlineScriptTool, + SkillScriptResult, SkillToolset, } from '@google/adk'; import {describe, expect, it, vi} from 'vitest'; +import {materializeScriptOutputs} from '../../../src/tools/skill/script_output_utils.js'; import {ToolConfirmation} from '../../../src/tools/tool_confirmation.js'; -import {materializeFiles} from '../../../src/utils/file_utils.js'; -vi.mock('../../../src/utils/file_utils.js', () => ({ - materializeFiles: vi.fn().mockImplementation((files) => files), +vi.mock('../../../src/tools/skill/script_output_utils.js', () => ({ + materializeScriptOutputs: vi + .fn() + .mockImplementation((result: CodeExecutionResult) => result), })); class MockCodeExecutor extends BaseCodeExecutor { @@ -51,6 +54,15 @@ interface ToolErrorResponse { errorCode: RunSkillInlineScriptErrorCode; } +function outputFile(): File { + return { + name: 'output.txt', + content: 'hello', + contentEncoding: FileContentEncoding.UTF8, + mimeType: 'text/plain', + }; +} + describe('RunSkillInlineScriptTool', () => { function createMockContext( agentName = 'test-agent', @@ -220,7 +232,7 @@ describe('RunSkillInlineScriptTool', () => { }); }); - it('calls materializeFiles with output files from executor', async () => { + it('materializes output files with no directory when none is configured', async () => { const mockExecutor = new MockCodeExecutor(); const testFile: File = { name: 'output.txt', @@ -247,7 +259,68 @@ describe('RunSkillInlineScriptTool', () => { }), }); - expect(materializeFiles).toHaveBeenCalledWith([testFile]); + expect(materializeScriptOutputs).toHaveBeenCalledWith( + mockExecutor.mockResult, + undefined, + ); + }); + + it('passes the toolset outputDir through', async () => { + const mockExecutor = new MockCodeExecutor(); + mockExecutor.mockResult = { + stdout: '', + stderr: '', + outputFiles: [outputFile()], + }; + + const toolset = new SkillToolset([], { + codeExecutor: mockExecutor, + outputDir: '/configured/dir', + }); + const tool = new RunSkillInlineScriptTool(toolset); + + await tool.runAsync({ + args: { + script_content: 'console.log("test");', + language: CodeExecutionLanguage.JAVASCRIPT, + }, + toolContext: createMockContext('test-agent', undefined, { + toolConfirmation: confirmed(), + }), + }); + + expect(materializeScriptOutputs).toHaveBeenCalledWith( + mockExecutor.mockResult, + '/configured/dir', + ); + }); + + it('returns the outputDir reported by the helper', async () => { + const mockExecutor = new MockCodeExecutor(); + mockExecutor.mockResult = { + stdout: '', + stderr: '', + outputFiles: [outputFile()], + }; + vi.mocked(materializeScriptOutputs).mockResolvedValueOnce({ + ...mockExecutor.mockResult, + outputDir: '/somewhere', + }); + + const toolset = new SkillToolset([], {codeExecutor: mockExecutor}); + const tool = new RunSkillInlineScriptTool(toolset); + + const result = (await tool.runAsync({ + args: { + script_content: 'console.log("test");', + language: CodeExecutionLanguage.JAVASCRIPT, + }, + toolContext: createMockContext('test-agent', undefined, { + toolConfirmation: confirmed(), + }), + })) as SkillScriptResult; + + expect(result.outputDir).toBe('/somewhere'); }); it('successfully passes array arguments to code executor', async () => { diff --git a/core/test/tools/skills/run_skill_script_tool_test.ts b/core/test/tools/skills/run_skill_script_tool_test.ts index 15c796b05..f9f170a06 100644 --- a/core/test/tools/skills/run_skill_script_tool_test.ts +++ b/core/test/tools/skills/run_skill_script_tool_test.ts @@ -11,17 +11,21 @@ import { Context, ExecuteCodeParams, File, + FileContentEncoding, InvocationContext, LlmAgent, RunSkillScriptTool, Skill, + SkillScriptResult, SkillToolset, } from '@google/adk'; import {describe, expect, it, vi} from 'vitest'; -import {materializeFiles} from '../../../src/utils/file_utils.js'; +import {materializeScriptOutputs} from '../../../src/tools/skill/script_output_utils.js'; -vi.mock('../../../src/utils/file_utils.js', () => ({ - materializeFiles: vi.fn(), +vi.mock('../../../src/tools/skill/script_output_utils.js', () => ({ + materializeScriptOutputs: vi + .fn() + .mockImplementation((result: CodeExecutionResult) => result), })); class MockCodeExecutor extends BaseCodeExecutor { @@ -204,28 +208,64 @@ describe('RunSkillScriptTool', () => { expect(binaryFile?.contentEncoding).toBe('base64'); }); - it('calls materializeFiles with output files from executor', async () => { - const mockExecutor = new MockCodeExecutor(); - const testFile = { - name: 'output.txt', - content: 'hello', - contentEncoding: 'utf8', - mimeType: 'text/plain', - } as File; - mockExecutor.mockResult = { - stdout: '', - stderr: '', - outputFiles: [testFile], - }; + const testFile: File = { + name: 'output.txt', + content: 'hello', + contentEncoding: FileContentEncoding.UTF8, + mimeType: 'text/plain', + }; - const toolset = new SkillToolset([mockSkill], {codeExecutor: mockExecutor}); - const tool = new RunSkillScriptTool(toolset); + function executorReturning(outputFiles: File[]): MockCodeExecutor { + const mockExecutor = new MockCodeExecutor(); + mockExecutor.mockResult = {stdout: '', stderr: '', outputFiles}; + return mockExecutor; + } - await tool.runAsync({ + async function runTool(toolset: SkillToolset): Promise { + return new RunSkillScriptTool(toolset).runAsync({ args: {skill_name: 'test-skill', script_path: 'scripts/setup.js'}, toolContext: createMockContext(), }); + } + + it('materializes output files with no directory when none is configured', async () => { + const mockExecutor = executorReturning([testFile]); + + await runTool(new SkillToolset([mockSkill], {codeExecutor: mockExecutor})); + + expect(materializeScriptOutputs).toHaveBeenCalledWith( + mockExecutor.mockResult, + undefined, + ); + }); + + it('passes the toolset outputDir through', async () => { + const mockExecutor = executorReturning([testFile]); + + await runTool( + new SkillToolset([mockSkill], { + codeExecutor: mockExecutor, + outputDir: '/configured/dir', + }), + ); + + expect(materializeScriptOutputs).toHaveBeenCalledWith( + mockExecutor.mockResult, + '/configured/dir', + ); + }); + + it('returns the outputDir reported by the helper', async () => { + const mockExecutor = executorReturning([testFile]); + vi.mocked(materializeScriptOutputs).mockResolvedValueOnce({ + ...mockExecutor.mockResult, + outputDir: '/somewhere', + }); + + const result = (await runTool( + new SkillToolset([mockSkill], {codeExecutor: mockExecutor}), + )) as SkillScriptResult; - expect(materializeFiles).toHaveBeenCalledWith([testFile]); + expect(result.outputDir).toBe('/somewhere'); }); }); diff --git a/core/test/tools/skills/script_output_utils_test.ts b/core/test/tools/skills/script_output_utils_test.ts new file mode 100644 index 000000000..c61123147 --- /dev/null +++ b/core/test/tools/skills/script_output_utils_test.ts @@ -0,0 +1,184 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import {CodeExecutionResult, File, FileContentEncoding} from '@google/adk'; +import * as fs from 'node:fs/promises'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import {afterEach, beforeEach, describe, expect, it} from 'vitest'; +import {materializeScriptOutputs} from '../../../src/tools/skill/script_output_utils.js'; + +/** + * Environment variables `os.tmpdir()` consults, so a test can give the + * implementation a temp root it exclusively owns and observe exactly what was + * created in it. POSIX reads TMPDIR; Windows reads TEMP then TMP. + */ +const TMPDIR_ENV_VARS = ['TMPDIR', 'TEMP', 'TMP'] as const; + +function textFile(name: string, content: string): File { + return { + name, + content, + contentEncoding: FileContentEncoding.UTF8, + mimeType: 'text/plain', + }; +} + +function executionResult(outputFiles: File[]): CodeExecutionResult { + return {stdout: 'out', stderr: 'err', outputFiles}; +} + +describe('materializeScriptOutputs', () => { + let tmpRoot: string; + let outputDir: string; + let originalTmpdirEnv: Array; + + beforeEach(async () => { + tmpRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'script_output_test_')); + // Real temp root is captured above, then redirected so the unconfigured + // path writes somewhere this test can enumerate and delete. + originalTmpdirEnv = TMPDIR_ENV_VARS.map((name) => process.env[name]); + for (const name of TMPDIR_ENV_VARS) { + process.env[name] = tmpRoot; + } + outputDir = await fs.mkdtemp(path.join(tmpRoot, 'configured_')); + }); + + afterEach(async () => { + TMPDIR_ENV_VARS.forEach((name, index) => { + const original = originalTmpdirEnv[index]; + if (original === undefined) { + delete process.env[name]; + } else { + process.env[name] = original; + } + }); + await fs.rm(tmpRoot, {recursive: true, force: true}); + }); + + it('writes output files into an explicit output directory', async () => { + const result = await materializeScriptOutputs( + executionResult([textFile('report.txt', 'contents')]), + outputDir, + ); + + expect(result.outputDir).toBe(outputDir); + expect(result.stdout).toBe('out'); + expect(result.stderr).toBe('err'); + expect(result.outputFiles.map((file) => file.name)).toEqual(['report.txt']); + expect(await fs.readFile(path.join(outputDir, 'report.txt'), 'utf8')).toBe( + 'contents', + ); + }); + + it('resolves a relative output directory against the working directory', async () => { + // Working directory is moved to the temp root rather than deriving a + // relative path from the real one: on Windows they sit on different + // drives, where no relative path between them exists. + const originalCwd = process.cwd(); + process.chdir(tmpRoot); + try { + const relativeDir = path.basename(outputDir); + expect(path.isAbsolute(relativeDir)).toBe(false); + const expectedDir = path.resolve(process.cwd(), relativeDir); + + const result = await materializeScriptOutputs( + executionResult([textFile('relative.txt', 'contents')]), + relativeDir, + ); + + expect(result.outputDir).toBe(expectedDir); + expect( + await fs.readFile(path.join(expectedDir, 'relative.txt'), 'utf8'), + ).toBe('contents'); + } finally { + process.chdir(originalCwd); + } + }); + + it('writes to a fresh temp directory when no output directory is configured', async () => { + const name = 'unconfigured_default_output.txt'; + + const result = await materializeScriptOutputs( + executionResult([textFile(name, 'contents')]), + ); + + if (!result.outputDir) { + expect.fail('expected an outputDir on the result'); + } + expect(path.dirname(result.outputDir)).toBe(tmpRoot); + expect(result.outputDir).not.toBe(process.cwd()); + expect(await fs.readFile(path.join(result.outputDir, name), 'utf8')).toBe( + 'contents', + ); + await expect(fs.access(path.join(process.cwd(), name))).rejects.toThrow( + /ENOENT/, + ); + }); + + it('creates a distinct directory per call', async () => { + const first = await materializeScriptOutputs( + executionResult([textFile('a.txt', 'first')]), + ); + const second = await materializeScriptOutputs( + executionResult([textFile('a.txt', 'second')]), + ); + + if (!first.outputDir || !second.outputDir) { + expect.fail('expected an outputDir on both results'); + } + expect(first.outputDir).not.toBe(second.outputDir); + expect(await fs.readFile(path.join(first.outputDir, 'a.txt'), 'utf8')).toBe( + 'first', + ); + expect( + await fs.readFile(path.join(second.outputDir, 'a.txt'), 'utf8'), + ).toBe('second'); + }); + + it('returns the result unchanged and creates nothing when there are no output files', async () => { + const before = await fs.readdir(tmpRoot); + const input = executionResult([]); + + const result = await materializeScriptOutputs(input); + + expect(result).toBe(input); + expect(result.outputDir).toBeUndefined(); + expect(await fs.readdir(tmpRoot)).toEqual(before); + }); + + it('rejects an output file that escapes the configured directory', async () => { + await expect( + materializeScriptOutputs( + executionResult([textFile(path.join('..', 'escape.txt'), 'nope')]), + outputDir, + ), + ).rejects.toThrow(/Path traversal detected/); + + await expect( + fs.access(path.resolve(outputDir, '..', 'escape.txt')), + ).rejects.toThrow(/ENOENT/); + }); + + it('appends a numeric suffix on collision within the configured directory', async () => { + await fs.writeFile(path.join(outputDir, 'notes.txt'), 'existing'); + + const result = await materializeScriptOutputs( + executionResult([textFile('notes.txt', 'fresh')]), + outputDir, + ); + + expect(result.outputFiles.map((file) => file.name)).toEqual([ + 'notes_2.txt', + ]); + expect(await fs.readFile(path.join(outputDir, 'notes.txt'), 'utf8')).toBe( + 'existing', + ); + expect(await fs.readFile(path.join(outputDir, 'notes_2.txt'), 'utf8')).toBe( + 'fresh', + ); + }); +}); diff --git a/core/test/tools/skills/skill_toolset_test.ts b/core/test/tools/skills/skill_toolset_test.ts index 8028a9ba2..9be002f1c 100644 --- a/core/test/tools/skills/skill_toolset_test.ts +++ b/core/test/tools/skills/skill_toolset_test.ts @@ -313,5 +313,17 @@ describe('skill_toolset', () => { expect(tools2.map((t) => t.name)).toContain('cached_tool'); expect(mockInnerGetTools).toHaveBeenCalledTimes(1); }); + + it('exposes the configured outputDir', () => { + const toolset = new SkillToolset([], {outputDir: '/tmp/skill-output'}); + + expect(toolset.outputDir).toBe('/tmp/skill-output'); + }); + + it('leaves outputDir undefined when not configured', () => { + const toolset = new SkillToolset([]); + + expect(toolset.outputDir).toBeUndefined(); + }); }); }); diff --git a/core/test/utils/file_utils_test.ts b/core/test/utils/file_utils_test.ts index ba44f8cc3..6e741d2f9 100644 --- a/core/test/utils/file_utils_test.ts +++ b/core/test/utils/file_utils_test.ts @@ -116,6 +116,45 @@ describe('file_utils', () => { expect(content2).toBe('world'); }); + it('rejects a sibling directory whose name extends the base directory name', async () => { + const base = path.join(tempDir, 'out'); + const files = [ + { + name: path.join('..', 'outX', 'leak.txt'), + content: 'dangerous', + contentEncoding: FileContentEncoding.UTF8, + mimeType: 'text/plain', + }, + ]; + + await expect(materializeFiles(files, base)).rejects.toThrow( + /Path traversal detected/, + ); + + await expect( + fs.access(path.join(tempDir, 'outX', 'leak.txt')), + ).rejects.toThrow(/ENOENT/); + }); + + it("allows a nested path whose segment merely starts with '..'", async () => { + const files = [ + { + name: path.join('..data', 'report.txt'), + content: 'contained', + contentEncoding: FileContentEncoding.UTF8, + mimeType: 'text/plain', + }, + ]; + + await materializeFiles(files, tempDir); + + const content = await fs.readFile( + path.join(tempDir, '..data', 'report.txt'), + 'utf8', + ); + expect(content).toBe('contained'); + }); + it('should append a numeric suffix to the filename if it already exists', async () => { const files = [ { diff --git a/output.txt b/output.txt deleted file mode 100644 index b6fc4c620..000000000 --- a/output.txt +++ /dev/null @@ -1 +0,0 @@ -hello \ No newline at end of file diff --git a/tests/integration/skills/script_js/agent.ts b/tests/integration/skills/script_js/agent.ts index d83a8869c..6dd90345e 100644 --- a/tests/integration/skills/script_js/agent.ts +++ b/tests/integration/skills/script_js/agent.ts @@ -37,6 +37,9 @@ export const rootAgent = new LlmAgent({ codeExecutor: new UnsafeLocalCodeExecutor(), // Inline-script execution is opt-in; enable it for this end-to-end test. allowInlineScripts: true, + // Script output goes only where the application declares. agent_test.ts + // creates a temporary directory and passes it in here. + outputDir: process.env['ADK_SKILL_OUTPUT_DIR'], }), ], // Executing model-provided inline scripts is gated behind a confirmation diff --git a/tests/integration/skills/script_js/agent_test.ts b/tests/integration/skills/script_js/agent_test.ts index b17df2c86..3d3da5e15 100644 --- a/tests/integration/skills/script_js/agent_test.ts +++ b/tests/integration/skills/script_js/agent_test.ts @@ -5,6 +5,8 @@ */ import {exec, spawn} from 'node:child_process'; import * as fs from 'node:fs/promises'; +import * as os from 'node:os'; +import * as path from 'node:path'; import {promisify} from 'node:util'; import {afterAll, beforeAll, describe, expect, it} from 'vitest'; import {normalizeLineEndings, sendInput} from '../../test_case_utils.js'; @@ -22,14 +24,21 @@ const TEST_EXECUTION_TIMEOUT = 60000; * 1. Starts the agent by running `npm run start` in the test project directory. * 2. Simulates user interaction by sending a prompt: "Let's create algorithmic art." * 3. Asserts that the agent's response matches the expected output, confirming it claims to have created the art and files. - * 4. Verifies that the expected files (`ephemeral_entanglement.md`, `index.html`, `sketch.js`) were actually generated in the file system. + * 4. Verifies that the expected files (`ephemeral_entanglement.md`, `index.html`, `sketch.js`) were generated in the output directory the agent was configured with, and not in its working directory. * 5. Compares the content of these generated files with reference files in the `expected/` directory to ensure correctness. - * 6. Cleans up the generated files and installed dependencies after execution. + * 6. Cleans up the output directory and installed dependencies after execution. + * + * The output directory is created here and handed to the agent through + * `ADK_SKILL_OUTPUT_DIR` (see `agent.ts`), because skill script output is only + * written to a directory the application declares. * * This test ensures the end-to-end flow of an agent using tools to generate and materialize files based on a high-level request. */ describe('Agent with skills that generates JS script and runs it locally', () => { + let outputDir: string; + beforeAll(async () => { + outputDir = await fs.mkdtemp(path.join(os.tmpdir(), 'adk-script-js-out-')); await execAsync('npm install', {cwd: PROJECT_PATH}); }, TEST_EXECUTION_TIMEOUT); @@ -39,6 +48,7 @@ describe('Agent with skills that generates JS script and runs it locally', () => const childProcess = spawn('npm', ['run', 'start'], { cwd: PROJECT_PATH, shell: true, + env: {...process.env, ADK_SKILL_OUTPUT_DIR: outputDir}, }); let response = await sendInput( @@ -52,20 +62,31 @@ describe('Agent with skills that generates JS script and runs it locally', () => response = await sendInput(childProcess, 'exit\n'); expect(response.toString()).toContain(''); - // verify that files were created and have the expected content + // verify that files were created in the declared output directory, and + // not in the agent's working directory, with the expected content const resultMdFile = await fs.readFile( - `${PROJECT_PATH}/ephemeral_entanglement.md`, + path.join(outputDir, 'ephemeral_entanglement.md'), 'utf-8', ); const resultScriptFile = await fs.readFile( - `${PROJECT_PATH}/sketch.js`, + path.join(outputDir, 'sketch.js'), 'utf-8', ); const resultHtmlFile = await fs.readFile( - `${PROJECT_PATH}/index.html`, + path.join(outputDir, 'index.html'), 'utf-8', ); + for (const name of [ + 'ephemeral_entanglement.md', + 'sketch.js', + 'index.html', + ]) { + await expect(fs.access(path.join(PROJECT_PATH, name))).rejects.toThrow( + /ENOENT/, + ); + } + const expectedMdFile = await fs.readFile( `${PROJECT_PATH}/expected/ephemeral_entanglement.md`, 'utf-8', @@ -93,12 +114,7 @@ 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(outputDir, {recursive: true, force: true}).catch(() => {}); await fs .rm(`${PROJECT_PATH}/node_modules`, {recursive: true, force: true}) diff --git a/tests/integration/tools/run_skill_inline_script_tool_test.ts b/tests/integration/tools/run_skill_inline_script_tool_test.ts index fcddc14d0..2f64709f0 100644 --- a/tests/integration/tools/run_skill_inline_script_tool_test.ts +++ b/tests/integration/tools/run_skill_inline_script_tool_test.ts @@ -10,15 +10,31 @@ import { Context, InvocationContext, RunSkillInlineScriptTool, + SkillScriptResult, SkillToolset, ToolConfirmation, UnsafeLocalCodeExecutor, } from '@google/adk'; import * as fs from 'node:fs/promises'; +import * as os from 'node:os'; import * as path from 'node:path'; -import {describe, expect, it} from 'vitest'; +import {afterEach, describe, expect, it} from 'vitest'; describe('RunSkillInlineScriptTool Integration with UnsafeLocalCodeExecutor', () => { + const scratchDirs: string[] = []; + + async function makeOutputDir(): Promise { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'adk-inline-out-')); + scratchDirs.push(dir); + return dir; + } + + afterEach(async () => { + while (scratchDirs.length > 0) { + await fs.rm(scratchDirs.pop()!, {recursive: true, force: true}); + } + }); + // These integration tests exercise real code execution, which is gated behind // a human-in-the-loop confirmation. Supply an already-confirmed confirmation // so the tool proceeds to execute (see run_skill_inline_script_tool.ts). @@ -138,9 +154,10 @@ describe('RunSkillInlineScriptTool Integration with UnsafeLocalCodeExecutor', () expect(result.stderr).toContain('some python error'); }); - it('creates files in process.cwd returned from execution', async () => { + it('writes output files into the configured outputDir', async () => { + const outputDir = await makeOutputDir(); const executor = new UnsafeLocalCodeExecutor(); - const toolset = new SkillToolset([], {codeExecutor: executor}); + const toolset = new SkillToolset([], {codeExecutor: executor, outputDir}); const tool = new RunSkillInlineScriptTool(toolset); const testFileName = `test_output_${Date.now()}.txt`; @@ -152,28 +169,20 @@ describe('RunSkillInlineScriptTool Integration with UnsafeLocalCodeExecutor', () language: CodeExecutionLanguage.JAVASCRIPT, }, toolContext: createMockContext(), - })) as CodeExecutionResult; - - expect(result).toBeDefined(); - expect(result.outputFiles).toBeDefined(); - expect(result.outputFiles?.length).toBeGreaterThan(0); + })) as SkillScriptResult; - const outputFile = result.outputFiles?.find((f) => f.name === testFileName); - expect(outputFile).toBeDefined(); + expect(result.outputDir).toBe(outputDir); + expect(result.outputFiles?.map((f) => f.name)).toContain(testFileName); - // Verify file was created in process.cwd() - const fullPath = path.join(process.cwd(), testFileName); - const exists = await fs - .access(fullPath) - .then(() => true) - .catch(() => false); - expect(exists).toBe(true); - - const content = await fs.readFile(fullPath, 'utf-8'); + const content = await fs.readFile( + path.join(outputDir, testFileName), + 'utf-8', + ); expect(content).toBe(testFileContent); - // Clean up - await fs.unlink(fullPath); + await expect( + fs.access(path.join(process.cwd(), testFileName)), + ).rejects.toThrow(/ENOENT/); }); it('successfully passes array arguments to a JavaScript inline script', async () => { @@ -213,16 +222,16 @@ describe('RunSkillInlineScriptTool Integration with UnsafeLocalCodeExecutor', () }); it('handles file collisions by appending a numeric suffix', async () => { + const outputDir = await makeOutputDir(); const executor = new UnsafeLocalCodeExecutor(); - const toolset = new SkillToolset([], {codeExecutor: executor}); + const toolset = new SkillToolset([], {codeExecutor: executor, outputDir}); const tool = new RunSkillInlineScriptTool(toolset); const testFileName = `test_inline_output_${Date.now()}.txt`; const testFileContent = 'hello from output file'; // Pre-create the target file to force a collision - const targetFile = path.join(process.cwd(), testFileName); - await fs.writeFile(targetFile, 'existing content'); + await fs.writeFile(path.join(outputDir, testFileName), 'existing content'); const result = (await tool.runAsync({ args: { @@ -230,30 +239,15 @@ describe('RunSkillInlineScriptTool Integration with UnsafeLocalCodeExecutor', () language: CodeExecutionLanguage.JAVASCRIPT, }, toolContext: createMockContext(), - })) as CodeExecutionResult; - - expect(result).toBeDefined(); - expect(result.outputFiles).toBeDefined(); - - const baseName = path.basename(testFileName, '.txt'); - const expectedName = `${baseName}_2.txt`; + })) as SkillScriptResult; - const outputFile = result.outputFiles?.find((f) => f.name === expectedName); - expect(outputFile).toBeDefined(); + const expectedName = `${path.basename(testFileName, '.txt')}_2.txt`; + expect(result.outputFiles?.map((f) => f.name)).toContain(expectedName); - // Verify collision file was created in process.cwd() - const fullPath = path.join(process.cwd(), expectedName); - const exists = await fs - .access(fullPath) - .then(() => true) - .catch(() => false); - expect(exists).toBe(true); - - const content = await fs.readFile(fullPath, 'utf-8'); + const content = await fs.readFile( + path.join(outputDir, expectedName), + 'utf-8', + ); expect(content).toBe(testFileContent); - - // Clean up both files - await fs.unlink(targetFile); - await fs.unlink(fullPath); }); }); diff --git a/tests/integration/tools/run_skill_script_tool_test.ts b/tests/integration/tools/run_skill_script_tool_test.ts index aa7f0ef6d..70967d3b7 100644 --- a/tests/integration/tools/run_skill_script_tool_test.ts +++ b/tests/integration/tools/run_skill_script_tool_test.ts @@ -10,13 +10,14 @@ import { InvocationContext, RunSkillScriptTool, Skill, + SkillScriptResult, SkillToolset, UnsafeLocalCodeExecutor, } from '@google/adk'; import * as fs from 'node:fs/promises'; import * as os from 'node:os'; import * as path from 'node:path'; -import {describe, expect, it} from 'vitest'; +import {afterEach, describe, expect, it} from 'vitest'; const IS_WINDOWS = os.platform() === 'win32'; const IS_UNIX = os.platform() === 'linux' || os.platform() === 'darwin'; @@ -28,6 +29,30 @@ const IS_UNIX = os.platform() === 'linux' || os.platform() === 'darwin'; const TEST_EXECUTION_TIMEOUT = 40000; describe('RunSkillScriptTool Integration with UnsafeLocalCodeExecutor', () => { + const scratchDirs: string[] = []; + + async function makeOutputDir(): Promise { + const dir = await fs.mkdtemp(path.join(os.tmpdir(), 'adk-script-out-')); + scratchDirs.push(dir); + return dir; + } + + /** + * Registers a directory the tool chose for removal. Asserting containment + * before registering keeps a wrong `outputDir` a test failure rather than a + * recursive delete of whatever the tool named. + */ + function trackToolOutputDir(dir: string): void { + expect(path.dirname(dir)).toBe(os.tmpdir()); + scratchDirs.push(dir); + } + + afterEach(async () => { + while (scratchDirs.length > 0) { + await fs.rm(scratchDirs.pop()!, {recursive: true, force: true}); + } + }); + function createMockContext(agentName = 'test-agent') { return new Context({ invocationContext: { @@ -280,9 +305,13 @@ describe('RunSkillScriptTool Integration with UnsafeLocalCodeExecutor', () => { TEST_EXECUTION_TIMEOUT, ); - it('creates files in process.cwd returned from execution', async () => { + it('writes output files into the configured outputDir', async () => { + const outputDir = await makeOutputDir(); const executor = new UnsafeLocalCodeExecutor(); - const toolset = new SkillToolset([testSkill], {codeExecutor: executor}); + const toolset = new SkillToolset([testSkill], { + codeExecutor: executor, + outputDir, + }); const tool = new RunSkillScriptTool(toolset); const result = (await tool.runAsync({ @@ -291,40 +320,67 @@ describe('RunSkillScriptTool Integration with UnsafeLocalCodeExecutor', () => { script_path: 'scripts/create_file.js', }, toolContext: createMockContext(), - })) as CodeExecutionResult; + })) as SkillScriptResult; - expect(result).toBeDefined(); - expect(result.outputFiles).toBeDefined(); - expect(result.outputFiles?.length).toBeGreaterThan(0); + expect(result.outputDir).toBe(outputDir); + expect(result.outputFiles?.map((f) => f.name)).toContain( + 'output_from_script.txt', + ); - const outputFile = result.outputFiles?.find( - (f) => f.name === 'output_from_script.txt', + const content = await fs.readFile( + path.join(outputDir, 'output_from_script.txt'), + 'utf-8', ); - expect(outputFile).toBeDefined(); + expect(content).toBe('hello from script file'); - // Verify file was created in process.cwd() - const fullPath = path.join(process.cwd(), 'output_from_script.txt'); - const exists = await fs - .access(fullPath) - .then(() => true) - .catch(() => false); - expect(exists).toBe(true); + await expect( + fs.access(path.join(process.cwd(), 'output_from_script.txt')), + ).rejects.toThrow(/ENOENT/); + }); - const content = await fs.readFile(fullPath, 'utf-8'); - expect(content).toBe('hello from script file'); + it('does not write output files into the working directory by default', async () => { + const executor = new UnsafeLocalCodeExecutor(); + const toolset = new SkillToolset([testSkill], {codeExecutor: executor}); + const tool = new RunSkillScriptTool(toolset); + + const result = (await tool.runAsync({ + args: { + skill_name: 'test-skill', + script_path: 'scripts/create_file.js', + }, + toolContext: createMockContext(), + })) as SkillScriptResult; + + if (!result.outputDir) { + expect.fail('expected an outputDir on the tool result'); + } + trackToolOutputDir(result.outputDir); + + await expect( + fs.access(path.join(process.cwd(), 'output_from_script.txt')), + ).rejects.toThrow(/ENOENT/); - // Clean up - await fs.unlink(fullPath); + const content = await fs.readFile( + path.join(result.outputDir, 'output_from_script.txt'), + 'utf-8', + ); + expect(content).toBe('hello from script file'); }); it('handles file collisions by appending a numeric suffix', async () => { + const outputDir = await makeOutputDir(); const executor = new UnsafeLocalCodeExecutor(); - const toolset = new SkillToolset([testSkill], {codeExecutor: executor}); + const toolset = new SkillToolset([testSkill], { + codeExecutor: executor, + outputDir, + }); const tool = new RunSkillScriptTool(toolset); // Pre-create the target file to force a collision - const targetFile = path.join(process.cwd(), 'output_from_script.txt'); - await fs.writeFile(targetFile, 'existing content'); + await fs.writeFile( + path.join(outputDir, 'output_from_script.txt'), + 'existing content', + ); const result = (await tool.runAsync({ args: { @@ -332,29 +388,16 @@ describe('RunSkillScriptTool Integration with UnsafeLocalCodeExecutor', () => { script_path: 'scripts/create_file.js', }, toolContext: createMockContext(), - })) as CodeExecutionResult; + })) as SkillScriptResult; - expect(result).toBeDefined(); - expect(result.outputFiles).toBeDefined(); - - const outputFile = result.outputFiles?.find( - (f) => f.name === 'output_from_script_2.txt', + expect(result.outputFiles?.map((f) => f.name)).toContain( + 'output_from_script_2.txt', ); - expect(outputFile).toBeDefined(); - - // Verify collision file was created in process.cwd() - const fullPath = path.join(process.cwd(), 'output_from_script_2.txt'); - const exists = await fs - .access(fullPath) - .then(() => true) - .catch(() => false); - expect(exists).toBe(true); - const content = await fs.readFile(fullPath, 'utf-8'); + const content = await fs.readFile( + path.join(outputDir, 'output_from_script_2.txt'), + 'utf-8', + ); expect(content).toBe('hello from script file'); - - // Clean up both files - await fs.unlink(targetFile); - await fs.unlink(fullPath); }); });