From 0c536882d54807be2ce2d1ebabd7c784b072718d Mon Sep 17 00:00:00 2001 From: Amaad Martin Date: Sun, 2 Aug 2026 18:52:24 -0700 Subject: [PATCH 1/5] Fix: contain skill script output in a declared directory instead of process.cwd() materializeFiles() defaulted its destination to process.cwd(), and both skill script tools called it with no directory, so script-chosen filenames were written into whichever directory the host process was launched from. Make dir a required parameter and route both tools through a new materializeScriptOutputs helper that writes under SkillToolset.outputDir when the application declares one, and otherwise into a fresh per-execution mkdtemp directory it reports back on the tool response. Also replace the raw startsWith() containment test with a path-segment comparison: with dir=/var/app/out, a name of ../outX/leak.txt resolved to /var/app/outX/leak.txt and passed the prefix check. That was latent while dir was the whole working directory and becomes load-bearing once outputDir names a small declared directory. Deletes the tracked root output.txt, which is an artifact this defect produced. --- core/src/common.ts | 1 + .../skill/run_skill_inline_script_tool.ts | 7 +- core/src/tools/skill/run_skill_script_tool.ts | 7 +- core/src/tools/skill/script_output_utils.ts | 64 +++++++ core/src/tools/skill/skill_toolset.ts | 18 ++ core/src/utils/file_utils.ts | 38 +++- .../run_skill_inline_script_tool_test.ts | 83 ++++++++- .../skills/run_skill_script_tool_test.ts | 79 ++++++-- .../tools/skills/script_output_utils_test.ts | 174 ++++++++++++++++++ core/test/tools/skills/skill_toolset_test.ts | 12 ++ core/test/utils/file_utils_test.ts | 39 ++++ output.txt | 1 - tests/integration/skills/script_js/agent.ts | 3 + .../skills/script_js/agent_test.ts | 40 ++-- .../run_skill_inline_script_tool_test.ts | 86 ++++----- .../tools/run_skill_script_tool_test.ts | 131 ++++++++----- 16 files changed, 640 insertions(+), 143 deletions(-) create mode 100644 core/src/tools/skill/script_output_utils.ts create mode 100644 core/test/tools/skills/script_output_utils_test.ts delete mode 100644 output.txt 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..2a31bd6d9 --- /dev/null +++ b/core/src/tools/skill/script_output_utils.ts @@ -0,0 +1,64 @@ +/** + * @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'; + +const OUTPUT_DIR_PREFIX = 'adk-skill-outputs-'; + +/** + * 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 to disk and reports + * where they went. + * + * With `outputDir` set, files are written under it (a relative path is + * resolved against the host process's working directory). Without it, a fresh + * directory is created for this execution under the OS temp directory, so + * script-chosen filenames never land in whichever directory the host process + * was launched from. Nothing is written and no directory is created when the + * script produced no output files. + * + * The directory is **not** cleaned up — it holds the artifacts the caller asked + * for. Unconfigured runs therefore rely on OS temp-directory cleanup; pass + * `outputDir` to put the files somewhere the application manages. + * + * @param result The result returned by the code executor. + * @param outputDir Directory to write the output files into. + * @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(), OUTPUT_DIR_PREFIX)); + + 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..50f8f9f77 100644 --- a/core/src/utils/file_utils.ts +++ b/core/src/utils/file_utils.ts @@ -9,19 +9,47 @@ 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. Required rather than defaulted: + * file names originate from script- or model-controlled data, and an + * implicit default writes them into whichever directory the host process + * happened to be launched from. + * @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 +79,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..a506de7a4 100644 --- a/core/test/tools/skills/run_skill_script_tool_test.ts +++ b/core/test/tools/skills/run_skill_script_tool_test.ts @@ -15,13 +15,16 @@ import { 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 +207,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 = { + name: 'output.txt', + content: 'hello', + contentEncoding: 'utf8', + mimeType: 'text/plain', + } as File; - 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..54741231f --- /dev/null +++ b/core/test/tools/skills/script_output_utils_test.ts @@ -0,0 +1,174 @@ +/** + * @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 to an absolute path', async () => { + const relativeDir = path.relative(process.cwd(), outputDir); + expect(path.isAbsolute(relativeDir)).toBe(false); + + const result = await materializeScriptOutputs( + executionResult([textFile('relative.txt', 'contents')]), + relativeDir, + ); + + expect(result.outputDir).toBe(outputDir); + expect( + await fs.readFile(path.join(outputDir, 'relative.txt'), 'utf8'), + ).toBe('contents'); + }); + + 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); }); }); From bfd9c2ff9edb8a2658a668e52d7e1c2ae6f7e044 Mon Sep 17 00:00:00 2001 From: Amaad Martin Date: Sun, 2 Aug 2026 18:56:46 -0700 Subject: [PATCH 2/5] Test: type the moved run_skill_script output-file fixture instead of casting The fixture hoisted out of the test body carried an 'as File' cast that tsc rejects (TS2352) because the string literal does not narrow to FileContentEncoding. Declare it as File with the enum member. --- core/test/tools/skills/run_skill_script_tool_test.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) 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 a506de7a4..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,6 +11,7 @@ import { Context, ExecuteCodeParams, File, + FileContentEncoding, InvocationContext, LlmAgent, RunSkillScriptTool, @@ -207,12 +208,12 @@ describe('RunSkillScriptTool', () => { expect(binaryFile?.contentEncoding).toBe('base64'); }); - const testFile = { + const testFile: File = { name: 'output.txt', content: 'hello', - contentEncoding: 'utf8', + contentEncoding: FileContentEncoding.UTF8, mimeType: 'text/plain', - } as File; + }; function executorReturning(outputFiles: File[]): MockCodeExecutor { const mockExecutor = new MockCodeExecutor(); From 8aec7df3c8a43d0bc2e0143e010a582c4677d18a Mon Sep 17 00:00:00 2001 From: Amaad Martin Date: Sun, 2 Aug 2026 19:11:03 -0700 Subject: [PATCH 3/5] Fix: make the relative-outputDir test portable across drives path.relative(process.cwd(), ) returns an absolute path on the Windows runner, where the working directory is on D: and the temp directory on C:, so the test's own precondition failed there. Move the working directory to the suite's temp root for the duration instead, which pins the resolution property directly and works on every platform. --- .../tools/skills/script_output_utils_test.ts | 36 ++++++++++++------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/core/test/tools/skills/script_output_utils_test.ts b/core/test/tools/skills/script_output_utils_test.ts index 54741231f..c61123147 100644 --- a/core/test/tools/skills/script_output_utils_test.ts +++ b/core/test/tools/skills/script_output_utils_test.ts @@ -74,19 +74,29 @@ describe('materializeScriptOutputs', () => { ); }); - it('resolves a relative output directory to an absolute path', async () => { - const relativeDir = path.relative(process.cwd(), outputDir); - expect(path.isAbsolute(relativeDir)).toBe(false); - - const result = await materializeScriptOutputs( - executionResult([textFile('relative.txt', 'contents')]), - relativeDir, - ); - - expect(result.outputDir).toBe(outputDir); - expect( - await fs.readFile(path.join(outputDir, 'relative.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 () => { From b2da46be0fe40a97d13b231f5eb5a3e2a7431abd Mon Sep 17 00:00:00 2001 From: Amaad Martin Date: Sun, 2 Aug 2026 19:35:57 -0700 Subject: [PATCH 4/5] Docs: state the output-directory lifetime caveat once, on the option The 'relative paths resolve against the working directory' and 'the directory is never deleted' caveats were repeated in three places. Keep them on SkillToolset's outputDir option, which is the public surface a user configures, and cut the internal helper's doc to a summary plus tags. materializeFiles's @param no longer argues why dir has no default: the signature says so. Also inlines the single-use OUTPUT_DIR_PREFIX constant. --- .foundry_review_brief.md.complexity | 183 ++++++++++++++++++++ core/src/tools/skill/script_output_utils.ts | 23 +-- core/src/utils/file_utils.ts | 5 +- 3 files changed, 190 insertions(+), 21 deletions(-) create mode 100644 .foundry_review_brief.md.complexity diff --git a/.foundry_review_brief.md.complexity b/.foundry_review_brief.md.complexity new file mode 100644 index 000000000..f38ef0a5d --- /dev/null +++ b/.foundry_review_brief.md.complexity @@ -0,0 +1,183 @@ +# Skill: Complexity Review (Simplicity Auditor) + +You are reviewing code changes for unnecessary complexity. Your goal is to +make the diff as short and simple as possible without sacrificing +correctness, security, or tests. You are a **read-only static reviewer** +spawned as a subagent by the Dev agent. + +## Instructions + +1. **Review the Diff**: Inspect the diff supplied to you (or run + `git diff main` / inspect the modified files in the current repo). + +2. **Identify Complexity**: Look for over-engineering, redundant code, dead + code, and unnecessary abstractions. + +3. **Do NOT Run Tests**: You are strictly performing a static review. Do NOT + execute tests or build steps. + +4. **Do NOT Modify Code**: You are only reviewing. Do NOT apply changes to the + source. + +5. **Output Format**: For each issue, write a comment as: + `L: . .` + (or `:L: ...` for multi-file reviews). + + Tags: + - `delete`: dead code, unused flexibility, speculative feature. Replacement: nothing. + - `stdlib`: hand-rolled thing the standard library ships. Name the function. + - `native`: dependency or code doing what the platform already does. Name the feature. + - `yagni`: abstraction with one implementation, config nobody sets, layer with one caller. + - `shrink`: same logic, fewer lines. Show the shorter form. + - `suppress`: the diff silences the type checker or linter instead of fixing + the type — `@ts-expect-error`, `@ts-ignore`, `eslint-disable`, `: any`, + `as any`, and equally: `as never`, `as unknown as T`, a + `[key: string]: any|unknown` index signature added to an SDK/request type, + a file-scope `/* eslint-disable */`, `/* v8 ignore start */` or any + coverage suppression, `catch (e: any)`, and `obj['privateField']` + string-index access used to reach a `private` member. Replacement: the real + type. Flag EVERY occurrence, in test files too. A generic reason string + (e.g. `// @ts-expect-error type fix`) is always a finding. A **file-scope** + disable is categorically worse than a single-line one — one line hides an + unbounded number of violations — so always flag it as blocking. Repetition + across files is a **blocking** finding: it means a signature is wrong + upstream and is being papered over at each call site — say so and name the + root signature to fix. + - `unrelated`: the file or hunk has nothing to do with the task — + `CHANGELOG.md` edits (release-please owns those), `package.json` / + `package-lock.json` version-bump churn with no real dependency change, + committed `*.patch` / `*.diff` / `*.orig` / `*.rej` artifacts, or scratch + scripts. Replacement: drop the hunk. If several such files appear together + it is a bad rebase — say so and tell the Dev agent to rebase cleanly onto + `main` rather than hand-deleting hunks. + - `placement`: the code is fine but lives in the wrong file. Two shapes: + (a) a cluster of constants + helper functions serving one concern + (formatting, parsing, conversion, truncation) inlined at the top of a + feature/class file instead of its own module — flag it once the cluster is + more than a couple of helpers or dominates the file's diff; and (b) a + genuinely reusable helper co-located under its feature directory, or named + with a feature prefix, instead of sitting in `core/src/utils/` with a + generic name. Ask "could another module plausibly want this?" — if yes it + belongs in shared utils as `_utils.ts`, NOT + `__utils.ts`. Co-location is only for helpers meaningless + outside their feature (`auth/oauth2/oauth2_utils.ts`). Also flag a moved + module that still hardcodes its first caller (feature-specific doc comments + or log prefixes), and a renamed module whose test file did not move with it. + Replacement: name the destination path and the generic module name. + Real case: `google/adk-js#527` needed two rounds of review — once for + inlining ~145 lines of error helpers into `mcp_session_manager.ts`, then + again because the extraction landed as `tools/mcp/mcp_error_utils.ts` + instead of `utils/error_utils.ts`. + + - `unjustified`: an abstraction whose only defence is "parity with + adk-python" — a wrapper type, a callback layer, a config object, an + indirection with exactly one caller inside the diff. Parity is a reason to + match *observable behaviour*; it is never a licence to import a shape the + JS runtime does not need. Replacement: name the concrete caller that + requires it, or delete it and call the underlying thing directly. Real + case: a `Task` wrapper defended as "asyncio.Task parity" drew *"What is the + purpose of that Task object?"* and then *"Can you please show an example of + the real usage?"* — no example existed and the PR stalled. Apply the same + test to any `?`-optional parameter that no caller actually omits. + - `regression`: the diff makes new code work by weakening an existing + guarantee — deleting a `throw`, loosening a validation, widening a + `private` for a test, relaxing an assertion, or swallowing an error that + used to propagate. This is the runtime twin of `suppress` and is always + blocking. Replacement: fix the cause upstream so the guarantee still holds. + Verbatim rejection: *"Seems dangerous to remove a thrown error and just + silently drop function events. Instead of fixing the new compaction here, + the compaction should properly adjust history so it does not happen."* + + **`suppress`, `unrelated`, `placement`, `unjustified` and `regression` are + correctness/hygiene gates, not complexity nits: never return `Lean already. + Ship.` while any remain unaddressed**, even if the diff is otherwise minimal. + + ### Sweep every finding across the whole diff + + When you find an instance of a pattern, **grep the whole diff for it and + report the count and all locations in ONE finding** — do not file N + near-identical comments, and never report only the first hit. Both ADK + maintainers state this outright and expect the same discipline back: + *"I won't comment on this again but it should be changed everywhere"*, + *"I will stop writing comments on every unknown but they should all be + known"*, and a bare *"here and everywhere else"* appearing in 8 comments + across 6 PRs. + + A finding fixed only at the line you cited comes straight back next round. + Write it as: `:L: (N occurrences: fileA:L12, + fileB:L40, ...). , applied to all N.` + + Repetition is diagnostic, not merely tedious: the same cast or guard at many + call sites means one signature is wrong upstream — name that root cause + rather than the symptoms. + + ### Tests are OUT OF SCOPE for `delete` and `shrink` + + **Never recommend removing, merging, or thinning a test case.** Test code is + not the complexity you are hunting: redundancy between tests is deliberate, + and a test that looks like it duplicates another usually pins a different + state combination. Coverage percentage does not prove otherwise — an audit + found 11 tests all passing against an injected bug at 100% branch coverage. + + This is not hypothetical: a Dev agent acting on a `delete`-tagged finding cut + 4 test cases (118 lines) from a PR during a review-fix pass, the only net + coverage loss across ten sibling PRs. That was a regression, and it came from + this rubric not saying otherwise. + + You may still flag, in test files: a `suppress` violation, a `.only`/`.skip` + left behind, `console.log` noise, or an assertion that cannot fail. Those are + correctness findings, not complexity ones. Everything else in a test file: + leave it alone. + + One more test finding, and it is the mirror image of the rule above: **flag + any hunk that EDITS an existing test rather than adding a new one.** Rewriting + a test's fixtures or assertions to accommodate new behaviour destroys the + regression signal that test was protecting, and the reviewer cannot tell + whether the old assertion was wrong or merely inconvenient. Tag it `regression` + and ask for a new case alongside the untouched original. One PR was asked this + three separate times: *"can you please create new test instead of modifying + existing one."* The legitimate exception — an existing test that genuinely + pinned wrong behaviour — must be called out explicitly in the PR body, not + slipped in. + + Likewise never propose deleting input validation at a trust boundary, error + handling that prevents data loss, a cleanup path (`finally`, listener/timer + teardown), a security check, or an accessibility affordance. "Fewer lines" is + not a reason to drop any of those. + +6. **Scoring**: End your review with: `net: - lines possible.` + +7. **Completion**: If there are NO findings, write EXACTLY this (including the + header, the note, and the full template block): + +``` +Lean already. Ship. +The Dev agent should now finalize the PR body (`.pr_body.md`) in the target repo root using the template below EXACTLY -- do not omit sections, change headers, or alter the checkbox options -- and then stage the PR on the developer's fork with: +`gh pr create --repo / --base main --head "" --title "" --body-file .pr_body.md`. +PR Body Template: +Please ensure you have read the contribution guide before creating a pull request. +### Link to Issue or Description of Change +1. Link to an existing issue (if applicable): +Closes: #issue_number +Related: #issue_number +2. Or, if no issue exists, describe the change: +**Problem**: A clear and concise description of what the problem is. +**Solution**: A clear and concise description of what you want to happen and why you choose this solution. +### Testing Plan +Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes. +Unit Tests: + [ ] I have added or updated unit tests for my change. + [ ] All unit tests pass locally. +Manual End-to-End (E2E) Tests: +Please provide instructions on how to manually test your changes, including any necessary setup or configuration. +### Checklist + [ ] I have read the CONTRIBUTING.md document. + [ ] I have performed a self-review of my own code. + [ ] I have commented my code, particularly in hard-to-understand areas. + [ ] I have added tests that prove my fix is effective or that my feature works. + [ ] New and existing unit tests pass locally with my changes. +``` + +8. **Persistence**: Write your full output to `.audit_comments.md` in the root + of the target repository (overwrite if it exists), AND return it as your + final message so the Dev agent receives your verdict directly. diff --git a/core/src/tools/skill/script_output_utils.ts b/core/src/tools/skill/script_output_utils.ts index 2a31bd6d9..bf6b93fff 100644 --- a/core/src/tools/skill/script_output_utils.ts +++ b/core/src/tools/skill/script_output_utils.ts @@ -10,8 +10,6 @@ import * as path from 'node:path'; import {CodeExecutionResult} from '../../code_executors/code_execution_utils.js'; import {materializeFiles} from '../../utils/file_utils.js'; -const OUTPUT_DIR_PREFIX = 'adk-skill-outputs-'; - /** * The result of a skill script execution, annotated with the directory its * output files were written to. @@ -25,22 +23,13 @@ export interface SkillScriptResult extends CodeExecutionResult { } /** - * Writes the output files of a skill script execution to disk and reports - * where they went. - * - * With `outputDir` set, files are written under it (a relative path is - * resolved against the host process's working directory). Without it, a fresh - * directory is created for this execution under the OS temp directory, so - * script-chosen filenames never land in whichever directory the host process - * was launched from. Nothing is written and no directory is created when the - * script produced no output files. - * - * The directory is **not** cleaned up — it holds the artifacts the caller asked - * for. Unconfigured runs therefore rely on OS temp-directory cleanup; pass - * `outputDir` to put the files somewhere the application manages. + * 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. + * @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. */ @@ -54,7 +43,7 @@ export async function materializeScriptOutputs( const dir = outputDir ? path.resolve(outputDir) - : await fs.mkdtemp(path.join(os.tmpdir(), OUTPUT_DIR_PREFIX)); + : await fs.mkdtemp(path.join(os.tmpdir(), 'adk-skill-outputs-')); return { ...result, diff --git a/core/src/utils/file_utils.ts b/core/src/utils/file_utils.ts index 50f8f9f77..1b956a786 100644 --- a/core/src/utils/file_utils.ts +++ b/core/src/utils/file_utils.ts @@ -33,10 +33,7 @@ function isContained(baseDir: string, fullPath: string): boolean { * * @param files The files to materialize. `name` is updated in place when a * collision forces a rename. - * @param dir Base directory to write under. Required rather than defaulted: - * file names originate from script- or model-controlled data, and an - * implicit default writes them into whichever directory the host process - * happened to be launched from. + * @param dir Base directory to write under. * @returns The written files, each `name` rewritten to the final path relative * to `dir`. */ From e988fbaac28a85ff9e0c58eb1053977a2b44df73 Mon Sep 17 00:00:00 2001 From: Amaad Martin Date: Sun, 2 Aug 2026 20:03:44 -0700 Subject: [PATCH 5/5] Chore: drop an accidentally committed review-harness scratch file .foundry_review_brief.md.complexity is tooling metadata that a git add -A swept in; it is not part of this change. --- .foundry_review_brief.md.complexity | 183 ---------------------------- 1 file changed, 183 deletions(-) delete mode 100644 .foundry_review_brief.md.complexity diff --git a/.foundry_review_brief.md.complexity b/.foundry_review_brief.md.complexity deleted file mode 100644 index f38ef0a5d..000000000 --- a/.foundry_review_brief.md.complexity +++ /dev/null @@ -1,183 +0,0 @@ -# Skill: Complexity Review (Simplicity Auditor) - -You are reviewing code changes for unnecessary complexity. Your goal is to -make the diff as short and simple as possible without sacrificing -correctness, security, or tests. You are a **read-only static reviewer** -spawned as a subagent by the Dev agent. - -## Instructions - -1. **Review the Diff**: Inspect the diff supplied to you (or run - `git diff main` / inspect the modified files in the current repo). - -2. **Identify Complexity**: Look for over-engineering, redundant code, dead - code, and unnecessary abstractions. - -3. **Do NOT Run Tests**: You are strictly performing a static review. Do NOT - execute tests or build steps. - -4. **Do NOT Modify Code**: You are only reviewing. Do NOT apply changes to the - source. - -5. **Output Format**: For each issue, write a comment as: - `L: . .` - (or `:L: ...` for multi-file reviews). - - Tags: - - `delete`: dead code, unused flexibility, speculative feature. Replacement: nothing. - - `stdlib`: hand-rolled thing the standard library ships. Name the function. - - `native`: dependency or code doing what the platform already does. Name the feature. - - `yagni`: abstraction with one implementation, config nobody sets, layer with one caller. - - `shrink`: same logic, fewer lines. Show the shorter form. - - `suppress`: the diff silences the type checker or linter instead of fixing - the type — `@ts-expect-error`, `@ts-ignore`, `eslint-disable`, `: any`, - `as any`, and equally: `as never`, `as unknown as T`, a - `[key: string]: any|unknown` index signature added to an SDK/request type, - a file-scope `/* eslint-disable */`, `/* v8 ignore start */` or any - coverage suppression, `catch (e: any)`, and `obj['privateField']` - string-index access used to reach a `private` member. Replacement: the real - type. Flag EVERY occurrence, in test files too. A generic reason string - (e.g. `// @ts-expect-error type fix`) is always a finding. A **file-scope** - disable is categorically worse than a single-line one — one line hides an - unbounded number of violations — so always flag it as blocking. Repetition - across files is a **blocking** finding: it means a signature is wrong - upstream and is being papered over at each call site — say so and name the - root signature to fix. - - `unrelated`: the file or hunk has nothing to do with the task — - `CHANGELOG.md` edits (release-please owns those), `package.json` / - `package-lock.json` version-bump churn with no real dependency change, - committed `*.patch` / `*.diff` / `*.orig` / `*.rej` artifacts, or scratch - scripts. Replacement: drop the hunk. If several such files appear together - it is a bad rebase — say so and tell the Dev agent to rebase cleanly onto - `main` rather than hand-deleting hunks. - - `placement`: the code is fine but lives in the wrong file. Two shapes: - (a) a cluster of constants + helper functions serving one concern - (formatting, parsing, conversion, truncation) inlined at the top of a - feature/class file instead of its own module — flag it once the cluster is - more than a couple of helpers or dominates the file's diff; and (b) a - genuinely reusable helper co-located under its feature directory, or named - with a feature prefix, instead of sitting in `core/src/utils/` with a - generic name. Ask "could another module plausibly want this?" — if yes it - belongs in shared utils as `_utils.ts`, NOT - `__utils.ts`. Co-location is only for helpers meaningless - outside their feature (`auth/oauth2/oauth2_utils.ts`). Also flag a moved - module that still hardcodes its first caller (feature-specific doc comments - or log prefixes), and a renamed module whose test file did not move with it. - Replacement: name the destination path and the generic module name. - Real case: `google/adk-js#527` needed two rounds of review — once for - inlining ~145 lines of error helpers into `mcp_session_manager.ts`, then - again because the extraction landed as `tools/mcp/mcp_error_utils.ts` - instead of `utils/error_utils.ts`. - - - `unjustified`: an abstraction whose only defence is "parity with - adk-python" — a wrapper type, a callback layer, a config object, an - indirection with exactly one caller inside the diff. Parity is a reason to - match *observable behaviour*; it is never a licence to import a shape the - JS runtime does not need. Replacement: name the concrete caller that - requires it, or delete it and call the underlying thing directly. Real - case: a `Task` wrapper defended as "asyncio.Task parity" drew *"What is the - purpose of that Task object?"* and then *"Can you please show an example of - the real usage?"* — no example existed and the PR stalled. Apply the same - test to any `?`-optional parameter that no caller actually omits. - - `regression`: the diff makes new code work by weakening an existing - guarantee — deleting a `throw`, loosening a validation, widening a - `private` for a test, relaxing an assertion, or swallowing an error that - used to propagate. This is the runtime twin of `suppress` and is always - blocking. Replacement: fix the cause upstream so the guarantee still holds. - Verbatim rejection: *"Seems dangerous to remove a thrown error and just - silently drop function events. Instead of fixing the new compaction here, - the compaction should properly adjust history so it does not happen."* - - **`suppress`, `unrelated`, `placement`, `unjustified` and `regression` are - correctness/hygiene gates, not complexity nits: never return `Lean already. - Ship.` while any remain unaddressed**, even if the diff is otherwise minimal. - - ### Sweep every finding across the whole diff - - When you find an instance of a pattern, **grep the whole diff for it and - report the count and all locations in ONE finding** — do not file N - near-identical comments, and never report only the first hit. Both ADK - maintainers state this outright and expect the same discipline back: - *"I won't comment on this again but it should be changed everywhere"*, - *"I will stop writing comments on every unknown but they should all be - known"*, and a bare *"here and everywhere else"* appearing in 8 comments - across 6 PRs. - - A finding fixed only at the line you cited comes straight back next round. - Write it as: `:L: (N occurrences: fileA:L12, - fileB:L40, ...). , applied to all N.` - - Repetition is diagnostic, not merely tedious: the same cast or guard at many - call sites means one signature is wrong upstream — name that root cause - rather than the symptoms. - - ### Tests are OUT OF SCOPE for `delete` and `shrink` - - **Never recommend removing, merging, or thinning a test case.** Test code is - not the complexity you are hunting: redundancy between tests is deliberate, - and a test that looks like it duplicates another usually pins a different - state combination. Coverage percentage does not prove otherwise — an audit - found 11 tests all passing against an injected bug at 100% branch coverage. - - This is not hypothetical: a Dev agent acting on a `delete`-tagged finding cut - 4 test cases (118 lines) from a PR during a review-fix pass, the only net - coverage loss across ten sibling PRs. That was a regression, and it came from - this rubric not saying otherwise. - - You may still flag, in test files: a `suppress` violation, a `.only`/`.skip` - left behind, `console.log` noise, or an assertion that cannot fail. Those are - correctness findings, not complexity ones. Everything else in a test file: - leave it alone. - - One more test finding, and it is the mirror image of the rule above: **flag - any hunk that EDITS an existing test rather than adding a new one.** Rewriting - a test's fixtures or assertions to accommodate new behaviour destroys the - regression signal that test was protecting, and the reviewer cannot tell - whether the old assertion was wrong or merely inconvenient. Tag it `regression` - and ask for a new case alongside the untouched original. One PR was asked this - three separate times: *"can you please create new test instead of modifying - existing one."* The legitimate exception — an existing test that genuinely - pinned wrong behaviour — must be called out explicitly in the PR body, not - slipped in. - - Likewise never propose deleting input validation at a trust boundary, error - handling that prevents data loss, a cleanup path (`finally`, listener/timer - teardown), a security check, or an accessibility affordance. "Fewer lines" is - not a reason to drop any of those. - -6. **Scoring**: End your review with: `net: - lines possible.` - -7. **Completion**: If there are NO findings, write EXACTLY this (including the - header, the note, and the full template block): - -``` -Lean already. Ship. -The Dev agent should now finalize the PR body (`.pr_body.md`) in the target repo root using the template below EXACTLY -- do not omit sections, change headers, or alter the checkbox options -- and then stage the PR on the developer's fork with: -`gh pr create --repo / --base main --head "" --title "" --body-file .pr_body.md`. -PR Body Template: -Please ensure you have read the contribution guide before creating a pull request. -### Link to Issue or Description of Change -1. Link to an existing issue (if applicable): -Closes: #issue_number -Related: #issue_number -2. Or, if no issue exists, describe the change: -**Problem**: A clear and concise description of what the problem is. -**Solution**: A clear and concise description of what you want to happen and why you choose this solution. -### Testing Plan -Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes. -Unit Tests: - [ ] I have added or updated unit tests for my change. - [ ] All unit tests pass locally. -Manual End-to-End (E2E) Tests: -Please provide instructions on how to manually test your changes, including any necessary setup or configuration. -### Checklist - [ ] I have read the CONTRIBUTING.md document. - [ ] I have performed a self-review of my own code. - [ ] I have commented my code, particularly in hard-to-understand areas. - [ ] I have added tests that prove my fix is effective or that my feature works. - [ ] New and existing unit tests pass locally with my changes. -``` - -8. **Persistence**: Write your full output to `.audit_comments.md` in the root - of the target repository (overwrite if it exists), AND return it as your - final message so the Dev agent receives your verdict directly.