Fix: require an explicit output directory and resolve the default in SkillToolset

core/src/tools/skill/skill_toolset.ts

@@ -47,7 +47,7 @@ export class SkillToolset extends BaseToolset {
   public registry?: SkillRegistry;
   private toolCache = new Map<string, BaseTool[]>();
   private fetchedSkillCache = new Map<string, Map<string, Skill>>();
-  public readonly outputDir?: string;
+  private readonly configuredOutputDir?: string;
 
   constructor(
     skills: Record<string, Skill> | 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<BaseTool[]> {
     const dynamicTools = await this.resolveAdditionalTools(context);
     return [...this.tools, ...dynamicTools];

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<File[]> {
   const resolvedBaseDir = path.resolve(dir);
   const createdFiles: File[] = [];

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}`,

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}`,

core/test/tools/skills/skill_toolset_test.ts

@@ -99,17 +99,41 @@ 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', () => {
         const outputDir = path.join(os.tmpdir(), 'skill-output');
         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 () => {

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.

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,33 +328,33 @@ 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);
     } finally {
       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,
       });
     }