Skip to content

refactor(core): create the unsafe executor's temp directory with mkdtemp #600

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kalenkevich merged 2 commits into from
Aug 4, 2026
+40 −5
Merged

refactor(core): create the unsafe executor's temp directory with mkdtemp #600

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6a99be7
refactor(core): create the unsafe executor's temp directory with mkdtemp
adilburaksen Aug 1, 2026
ec69431
refactor(core): address review nits on the mkdtemp change
adilburaksen Aug 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions core/src/code_executors/unsafe_local_code_executor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,13 @@ async function createTempScriptFile(
language: CodeExecutionLanguage,
shellCommandPath?: string,
): Promise<{filePath: string; tempDir: string}> {
const tempDir = path.join(
os.tmpdir(),
'adk_js_unsafe_code_executor',
Date.now().toString() + '_' + Math.random().toString(36).slice(2),
// mkdtemp creates the directory itself, exclusively and mode 0o700. The
// previous Date.now()/Math.random() name was predictable, and mkdir with
// `recursive` does not fail on a path that already exists, so a directory
// pre-created by another local user was adopted rather than rejected.

@AmaadMartin AmaadMartin Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit. The comment narrates the old code rather than the new.

  // mkdtemp creates the directory itself, exclusively and mode 0o700. The
  // previous Date.now()/Math.random() name was predictable, and mkdir with
  // `recursive` does not fail on a path that already exists, so a directory
  // pre-created by another local user was adopted rather than rejected.
  // mkdtemp names the directory itself and creates it exclusively at 0o700.

Nothing else in core/src explains a prior implementation (grepped — zero hits), and your PR description already covers the archaeology, which is where it survives a rename. Same shape as the doc-comment nit on #599, so treat it as one habit rather than two findings.

const tempDir = await fs.mkdtemp(
path.join(os.tmpdir(), 'adk_js_unsafe_code_executor_'),
);
await fs.mkdir(tempDir, {recursive: true});

const ext = getExtensionForLanguage(language, shellCommandPath) || '.js';
const filePath = path.join(tempDir, `script${ext}`);
Expand Down
32 changes: 32 additions & 0 deletions core/test/code_executors/unsafe_local_code_executor_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
} from '@google/adk';
import {EventEmitter} from 'node:events';
import * as os from 'node:os';
import * as path from 'node:path';
import {beforeEach, describe, expect, it, vi} from 'vitest';

// Only `spawn` is mocked; it defaults to the real implementation (see
Expand Down Expand Up @@ -90,6 +91,37 @@ describe('UnsafeLocalCodeExecutor', () => {
expect(result.stderr).toBe('');
});

// The script runs with the temporary directory as its cwd, so it can report
// the name and mode the executor actually created.
it('creates a private, unpredictable temporary directory', async () => {
const params: ExecuteCodeParams = {
invocationContext,
codeExecutionInput: {
code: [
'const fs = require("node:fs");',
'const dir = process.cwd();',
'const mode = (fs.statSync(dir).mode & 0o777).toString(8);',
'console.log(JSON.stringify({dir, mode}));',
].join('\n'),
language: CodeExecutionLanguage.JAVASCRIPT,
inputFiles: [],
},
};

const first = JSON.parse((await executor.executeCode(params)).stdout);
const second = JSON.parse((await executor.executeCode(params)).stdout);

@AmaadMartin AmaadMartin Aug 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit, optional. If either run fails, JSON.parse throws Unexpected end of JSON input and swallows the stderr that would explain why.

    const first = JSON.parse((await executor.executeCode(params)).stdout);
    const second = JSON.parse((await executor.executeCode(params)).stdout);
    const firstResult = await executor.executeCode(params);
    const secondResult = await executor.executeCode(params);
    expect(firstResult.stderr).toBe('');
    expect(secondResult.stderr).toBe('');

    const first = JSON.parse(firstResult.stdout);
    const second = JSON.parse(secondResult.stdout);

should execute code and return stdout at :91 already asserts stderr is '' for a plain node script, so the added assertion is one that passes today.


// mkdtemp appends six random characters to the prefix it is given.
expect(path.basename(first.dir)).toMatch(
/^adk_js_unsafe_code_executor_.{6}$/,
);
expect(second.dir).not.toBe(first.dir);

if (os.platform() !== 'win32') {
expect(first.mode).toBe('700');
}
});

it('should capture stderr', async () => {
const params: ExecuteCodeParams = {
invocationContext,
Expand Down
Loading