Skip to content

feat: Add environment tools matching Python parity - #555

Open
AmaadMartin wants to merge 3 commits into
google:mainfrom
AmaadMartin:feat/js-environment-tools
Open

feat: Add environment tools matching Python parity#555
AmaadMartin wants to merge 3 commits into
google:mainfrom
AmaadMartin:feat/js-environment-tools

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

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):

N/A — no existing issue tracks this work.

2. Or, if no issue exists, describe the change:

Problem:
The JS variant of ADK lacks parity with the adk-python environment tools, missing
necessary functionality for agent local file manipulation and shell execution.

Solution:
Added Node.js parity environment tools (ExecuteTool, ReadFileTool,
WriteFileTool, EditFileTool) and a bundled EnvironmentToolset in
core/src/tools/environment/. The toolset also injects the
ENVIRONMENT_INSTRUCTION into the LLM system requests for bounded contextual
usage.

One deliberate deviation from adk-python, please read:
ExecuteTool requires an explicit tool confirmation before it runs a command.
adk-python's execute tool has no such gate, but it also does not run anything
itself: it delegates to BaseEnvironment.execute(), so "run on the host" is one
backend an embedder selects. This port collapses that seam into a hardcoded
child_process implementation behind workingDir: string, which makes host
execution the only option — a model-authored string becomes arbitrary code
execution on the machine running the agent, and cwd is not a boundary. The gate
uses the same mechanism RunSkillInlineScriptTool already uses on main for
model-provided scripts. Restoring the BaseEnvironment seam would be the better
long-term answer and is a reasonable follow-up.

All four tool classes carry @experimental, matching adk-python and reflecting
that they became public API here via core/src/common.ts and core/src/index.ts.
Each tool exports its result type (ExecuteResult, ReadFileResult,
WriteFileResult, EditFileResult) rather than returning the base class's
Promise<unknown>.

There are no any, as any, eslint-disable, @ts-ignore or @ts-expect-error
uses anywhere in this diff, in src/ or in the tests.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.

Added core/test/tools/environment/tools_test.ts, covering boundaries across
reading, writing, editing and shell execution, plus the confirmation gate and
literal-text matching in EditFile. Local run:

$ npx vitest run --project unit:core core/test/tools/environment/tools_test.ts

 ✓ |unit:core| core/test/tools/environment/tools_test.ts (32 tests) 216ms

 Test Files  1 passed (1)
      Tests  32 passed (32)

Manual End-to-End (E2E) Tests:

  1. Build the workspace with npm run build.
  2. Run npx vitest run --project unit:core core/test/tools/environment/tools_test.ts
    to exercise the tools against a temporary working directory.

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.
  • I have manually tested my changes end-to-end.
  • Any dependent changes have been merged and published in downstream modules.

Additional context

resolveAndValidatePath performs a lexical containment check. It rejects ..
and absolute-path arguments, but path.resolve/path.relative never touch the
filesystem, so a symlink inside workingDir that points elsewhere still resolves
through. That is intentional — reading through symlinks is necessary for real
workspace layouts such as the package links npm creates under node_modules — and
the docstring says so explicitly. It is not a sandbox; callers who need one should
point the tools at an isolated filesystem.

This branch also carries one small, independent fix commit:
fix(dev): output agent bundle in project .adk_build_cache for hoisted dependencies,
which changes dev/src/utils/agent_loader.ts to emit the compiled agent bundle into
a project-local .adk_build_cache directory instead of the system temp directory, so
that hoisted node_modules dependencies resolve correctly. Happy to split this into a
separate PR if reviewers prefer.

@AmaadMartin
AmaadMartin force-pushed the feat/js-environment-tools branch 2 times, most recently from 5e87d4d to 4124503 Compare July 28, 2026 21:18
Amaad Martin added 3 commits July 28, 2026 18:53
Correctness:
- escapeRegExp escaped nothing: the character class closed early, so
  old_string was matched as a live regex. Metacharacters could match the
  wrong bytes and an unbalanced group threw out of runAsync instead of
  returning the {status: 'error'} contract.
- EditFile expanded $&, $` and $1 in new_string. Use a function
  replacement so the text is inserted literally.
- The truncation notice reported the original length as if it were the
  limit ("truncated to 10 chars" for 5 characters of content).

Types (no suppressions anywhere in the diff):
- Each tool declares and exports its result type (ExecuteResult,
  ReadFileResult, WriteFileResult, EditFileResult), narrowing the
  Promise<unknown> from BaseTool. Callers and tests are typed with no
  annotations.
- catch blocks bind unknown and narrow through toError /
  isFileNotFoundError instead of `catch (e: any)`; the exec failure path
  uses child_process.ExecException, which already declares killed, code,
  stdout and stderr.
- isValidLineNumber is a module-level type predicate, replacing an inline
  closure, a dead !isNaN check and two unchecked casts on unknown.

Security:
- Execute now passes through the standard tool-confirmation gate before
  it shells out, matching how RunSkillInlineScriptTool gates
  model-provided code. workingDir is not a boundary for a shell command,
  and these tools are public API.

Also: @experimental on all four tool classes (they are public API and
adk-python decorates all four), drop the leading underscore from the
description constant, and document that path containment is lexical and
does not resolve symlinks.
@AmaadMartin
AmaadMartin force-pushed the feat/js-environment-tools branch from 4124503 to c3360f4 Compare July 29, 2026 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant