diff --git a/core/src/tools/skill/skill_toolset.ts b/core/src/tools/skill/skill_toolset.ts index a6054262b..5fd664100 100644 --- a/core/src/tools/skill/skill_toolset.ts +++ b/core/src/tools/skill/skill_toolset.ts @@ -47,7 +47,7 @@ export class SkillToolset extends BaseToolset { public registry?: SkillRegistry; private toolCache = new Map(); private fetchedSkillCache = new Map>(); - public readonly outputDir?: string; + private readonly configuredOutputDir?: string; constructor( skills: Record | Skill[], @@ -65,12 +65,10 @@ export class SkillToolset extends BaseToolset { */ allowInlineScripts?: boolean; /** - * Directory that skill script output files are written into. The names - * the tools report back to the model are relative to it. - * - * Defaults to the host process's current working directory, so an agent - * launched from a source checkout writes model-named files into that - * checkout; set this to keep skill output out of the working tree. + * Directory that files produced by `run_skill_script` and + * `run_skill_inline_script` are written to. Relative paths resolve + * against the agent process's working directory. Defaults to the agent + * process's current working directory. */ outputDir?: string; } = {}, @@ -82,7 +80,7 @@ export class SkillToolset extends BaseToolset { this.codeExecutor = options.codeExecutor; this.additionalTools = options.additionalTools || []; this.registry = options.registry; - this.outputDir = options.outputDir; + this.configuredOutputDir = options.outputDir; this.tools = [ new ListSkillsTool(this), @@ -102,6 +100,14 @@ export class SkillToolset extends BaseToolset { } } + /** + * Resolved per read, so a host that changes its working directory is not + * pinned to the value captured at construction. + */ + get outputDir(): string { + return this.configuredOutputDir ?? process.cwd(); + } + override async getTools(context?: ReadonlyContext): Promise { const dynamicTools = await this.resolveAdditionalTools(context); return [...this.tools, ...dynamicTools]; diff --git a/core/src/utils/file_utils.ts b/core/src/utils/file_utils.ts index e91b54b26..87a81abe1 100644 --- a/core/src/utils/file_utils.ts +++ b/core/src/utils/file_utils.ts @@ -9,24 +9,23 @@ import * as path from 'node:path'; import {File} from '../code_executors/code_execution_utils.js'; /** - * Writes the given in-memory files to disk under a base directory, appending a - * numeric suffix (`report.txt` -> `report_2.txt`) rather than overwriting an - * existing file. + * Writes the given files into `dir`, creating parent directories as needed and + * appending a `_2`, `_3`, ... suffix when a name is already taken. * - * Names resolving outside `dir` are rejected with a `Path traversal detected` - * error. That is a lexical check on the resolved path, not a sandbox: it does - * not survive symlinks or a concurrent rename. + * File names are constrained to `dir` by a lexical path comparison. That is a + * useful guard, not a sandbox: it does not survive symlinks, hardlinks, bind + * mounts, or TOCTOU races. * * @param files The files to materialize. - * @param dir Base directory to write under. Defaults to the host process's - * current working directory; callers that do not want files there must - * pass an explicit directory. - * @returns The written files, with `name` rewritten to the final path relative + * @param dir The directory the files are written into, required so a caller + * cannot silently fall back to the host process's working directory. A + * relative path resolves against the current working directory. + * @return The files as written, with `name` updated 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[] = []; 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 0264ffc84..8966843fc 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 @@ -228,16 +228,19 @@ describe('RunSkillInlineScriptTool', () => { }); }); - const testFile: File = { - name: 'output.txt', - content: 'hello', - contentEncoding: FileContentEncoding.UTF8, - mimeType: 'text/plain', - }; - it('calls materializeFiles with output files from executor', async () => { const mockExecutor = new MockCodeExecutor(); - mockExecutor.mockResult = {stdout: '', stderr: '', outputFiles: [testFile]}; + const testFile: File = { + name: 'output.txt', + content: 'hello', + contentEncoding: FileContentEncoding.UTF8, + mimeType: 'text/plain', + }; + mockExecutor.mockResult = { + stdout: '', + stderr: '', + outputFiles: [testFile], + }; const toolset = new SkillToolset([], {codeExecutor: mockExecutor}); const tool = new RunSkillInlineScriptTool(toolset); @@ -252,14 +255,24 @@ describe('RunSkillInlineScriptTool', () => { }), }); - // No configured directory: materializeFiles applies its own cwd default. - expect(materializeFiles).toHaveBeenCalledWith([testFile], undefined); + // No configured directory: the toolset resolves the cwd default itself. + expect(materializeFiles).toHaveBeenCalledWith([testFile], process.cwd()); }); it('materializes output files into the configured output directory', async () => { const outputDir = path.join(os.tmpdir(), 'skill-inline-output'); const mockExecutor = new MockCodeExecutor(); - mockExecutor.mockResult = {stdout: '', stderr: '', outputFiles: [testFile]}; + const testFile: File = { + name: 'output.txt', + content: 'hello', + contentEncoding: FileContentEncoding.UTF8, + mimeType: 'text/plain', + }; + mockExecutor.mockResult = { + stdout: '', + stderr: '', + outputFiles: [testFile], + }; const toolset = new SkillToolset([], { codeExecutor: mockExecutor, @@ -283,7 +296,17 @@ describe('RunSkillInlineScriptTool', () => { it('surfaces an EXECUTION_ERROR when materializing output files is refused', async () => { const outputDir = path.join(os.tmpdir(), 'skill-inline-output'); const mockExecutor = new MockCodeExecutor(); - mockExecutor.mockResult = {stdout: '', stderr: '', outputFiles: [testFile]}; + const testFile: File = { + name: 'output.txt', + content: 'hello', + contentEncoding: FileContentEncoding.UTF8, + mimeType: 'text/plain', + }; + mockExecutor.mockResult = { + stdout: '', + stderr: '', + outputFiles: [testFile], + }; vi.mocked(materializeFiles).mockRejectedValueOnce( new Error( `Path traversal detected: ../escape.txt resolves outside of ${outputDir}`, 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 846114f05..813eb0394 100644 --- a/core/test/tools/skills/run_skill_script_tool_test.ts +++ b/core/test/tools/skills/run_skill_script_tool_test.ts @@ -213,16 +213,19 @@ describe('RunSkillScriptTool', () => { expect(binaryFile?.contentEncoding).toBe('base64'); }); - const testFile: File = { - name: 'output.txt', - content: 'hello', - contentEncoding: FileContentEncoding.UTF8, - mimeType: 'text/plain', - }; - it('calls materializeFiles with output files from executor', async () => { const mockExecutor = new MockCodeExecutor(); - mockExecutor.mockResult = {stdout: '', stderr: '', outputFiles: [testFile]}; + const testFile: File = { + name: 'output.txt', + content: 'hello', + contentEncoding: FileContentEncoding.UTF8, + mimeType: 'text/plain', + }; + mockExecutor.mockResult = { + stdout: '', + stderr: '', + outputFiles: [testFile], + }; const toolset = new SkillToolset([mockSkill], {codeExecutor: mockExecutor}); const tool = new RunSkillScriptTool(toolset); @@ -232,14 +235,24 @@ describe('RunSkillScriptTool', () => { toolContext: createMockContext(), }); - // No configured directory: materializeFiles applies its own cwd default. - expect(materializeFiles).toHaveBeenCalledWith([testFile], undefined); + // No configured directory: the toolset resolves the cwd default itself. + expect(materializeFiles).toHaveBeenCalledWith([testFile], process.cwd()); }); it('materializes output files into the configured output directory', async () => { const outputDir = path.join(os.tmpdir(), 'skill-output'); const mockExecutor = new MockCodeExecutor(); - mockExecutor.mockResult = {stdout: '', stderr: '', outputFiles: [testFile]}; + const testFile: File = { + name: 'output.txt', + content: 'hello', + contentEncoding: FileContentEncoding.UTF8, + mimeType: 'text/plain', + }; + mockExecutor.mockResult = { + stdout: '', + stderr: '', + outputFiles: [testFile], + }; const toolset = new SkillToolset([mockSkill], { codeExecutor: mockExecutor, @@ -258,7 +271,17 @@ describe('RunSkillScriptTool', () => { it('surfaces an EXECUTION_ERROR when materializing output files is refused', async () => { const outputDir = path.join(os.tmpdir(), 'skill-output'); const mockExecutor = new MockCodeExecutor(); - mockExecutor.mockResult = {stdout: '', stderr: '', outputFiles: [testFile]}; + const testFile: File = { + name: 'output.txt', + content: 'hello', + contentEncoding: FileContentEncoding.UTF8, + mimeType: 'text/plain', + }; + mockExecutor.mockResult = { + stdout: '', + stderr: '', + outputFiles: [testFile], + }; vi.mocked(materializeFiles).mockRejectedValueOnce( new Error( `Path traversal detected: ../escape.txt resolves outside of ${outputDir}`, diff --git a/core/test/tools/skills/skill_toolset_test.ts b/core/test/tools/skills/skill_toolset_test.ts index 256ef4aa8..f2716a94a 100644 --- a/core/test/tools/skills/skill_toolset_test.ts +++ b/core/test/tools/skills/skill_toolset_test.ts @@ -99,10 +99,8 @@ describe('skill_toolset', () => { }); describe('outputDir', () => { - it('is undefined when no directory is configured', () => { - // The toolset deliberately resolves no default of its own, so the cwd - // default stays where it was: resolved per call by materializeFiles. - expect(new SkillToolset([mockSkill]).outputDir).toBeUndefined(); + it('defaults to the process working directory', () => { + expect(new SkillToolset([mockSkill]).outputDir).toBe(process.cwd()); }); it('exposes the configured directory', () => { @@ -110,6 +108,32 @@ describe('skill_toolset', () => { const toolset = new SkillToolset([mockSkill], {outputDir}); expect(toolset.outputDir).toBe(outputDir); }); + + it('resolves the working directory on each read, not at construction', () => { + const toolset = new SkillToolset([mockSkill]); + const movedTo = path.join(os.tmpdir(), 'skill-output-after-chdir'); + const cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(movedTo); + + try { + expect(toolset.outputDir).toBe(movedTo); + } finally { + cwdSpy.mockRestore(); + } + }); + + it('keeps a configured directory when the process working directory moves', () => { + const outputDir = path.join(os.tmpdir(), 'skill-output'); + const toolset = new SkillToolset([mockSkill], {outputDir}); + const cwdSpy = vi + .spyOn(process, 'cwd') + .mockReturnValue(path.join(os.tmpdir(), 'somewhere-else')); + + try { + expect(toolset.outputDir).toBe(outputDir); + } finally { + cwdSpy.mockRestore(); + } + }); }); it('appends instructions to LLM request', async () => { diff --git a/core/test/utils/file_utils_test.ts b/core/test/utils/file_utils_test.ts index f602d342f..923e93f6d 100644 --- a/core/test/utils/file_utils_test.ts +++ b/core/test/utils/file_utils_test.ts @@ -8,7 +8,7 @@ import {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, vi} from 'vitest'; +import {afterEach, beforeEach, describe, expect, it} from 'vitest'; import {materializeFiles} from '../../src/utils/file_utils.js'; describe('file_utils', () => { @@ -54,44 +54,6 @@ describe('file_utils', () => { expect(content2).toBe('world'); }); - it('should default the base directory to the working directory of each call', async () => { - // Callers that omit `dir` — the skill script tools when no output - // directory is configured — follow process.cwd() as of the call, not as - // of module load, so a process that chdir()s is tracked. - const secondDir = await fs.mkdtemp( - path.join(os.tmpdir(), 'file_utils_test_second_'), - ); - const newFile = () => [ - { - name: 'default_dir.txt', - content: 'hello', - contentEncoding: FileContentEncoding.UTF8, - mimeType: 'text/plain', - }, - ]; - const cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(tempDir); - - try { - const first = await materializeFiles(newFile()); - expect(first[0].name).toBe('default_dir.txt'); - - cwdSpy.mockReturnValue(secondDir); - await materializeFiles(newFile()); - - // Written under the cwd in effect at each call, not a single snapshot - // (a snapshot would have collided and produced default_dir_2.txt). - expect( - await fs.readFile(path.join(tempDir, 'default_dir.txt'), 'utf8'), - ).toBe('hello'); - expect( - await fs.readFile(path.join(secondDir, 'default_dir.txt'), 'utf8'), - ).toBe('hello'); - } finally { - cwdSpy.mockRestore(); - await fs.rm(secondDir, {recursive: true, force: true}); - } - }); - it('should create the target directory when it does not exist', async () => { // What a configured `outputDir` relies on: the operator names a // directory, the first write brings it into existence. diff --git a/tests/integration/tools/run_skill_script_tool_test.ts b/tests/integration/tools/run_skill_script_tool_test.ts index 4e8fa822b..0665e8537 100644 --- a/tests/integration/tools/run_skill_script_tool_test.ts +++ b/tests/integration/tools/run_skill_script_tool_test.ts @@ -60,6 +60,12 @@ describe('RunSkillScriptTool Integration with UnsafeLocalCodeExecutor', () => { 'create_file.js': { src: "const fs = require('fs'); fs.writeFileSync('output_from_script.txt', 'hello from script file');", }, + // A file name owned solely by the configured-output-dir test, so its + // cwd-absence assertion cannot be perturbed by the neighbouring tests + // that write, unlink and pre-create output_from_script.txt in the cwd. + 'create_file_for_output_dir.js': { + src: "const fs = require('fs'); fs.writeFileSync('output_to_configured_dir.txt', 'hello from script file');", + }, 'hello.ps1': { src: 'Write-Host "hello from skill powershell"', }, @@ -322,25 +328,25 @@ describe('RunSkillScriptTool Integration with UnsafeLocalCodeExecutor', () => { const result = (await tool.runAsync({ args: { skill_name: 'test-skill', - script_path: 'scripts/create_file.js', + script_path: 'scripts/create_file_for_output_dir.js', }, toolContext: createMockContext(), })) as CodeExecutionResult; const outputFile = result.outputFiles?.find( - (f) => f.name === 'output_from_script.txt', + (f) => f.name === 'output_to_configured_dir.txt', ); expect(outputFile).toBeDefined(); const content = await fs.readFile( - path.join(outputDir, 'output_from_script.txt'), + path.join(outputDir, 'output_to_configured_dir.txt'), 'utf-8', ); expect(content).toBe('hello from script file'); // The launch directory must stay clean. const inCwd = await fs - .access(path.join(process.cwd(), 'output_from_script.txt')) + .access(path.join(process.cwd(), 'output_to_configured_dir.txt')) .then(() => true) .catch(() => false); expect(inCwd).toBe(false); @@ -348,7 +354,7 @@ describe('RunSkillScriptTool Integration with UnsafeLocalCodeExecutor', () => { await fs.rm(outputDir, {recursive: true, force: true}); // A regression writes to the launch directory instead; remove it so a // failing run does not leave the working tree dirty. - await fs.rm(path.join(process.cwd(), 'output_from_script.txt'), { + await fs.rm(path.join(process.cwd(), 'output_to_configured_dir.txt'), { force: true, }); }