Feat: Port the environment abstraction (BaseEnvironment, ExecutionResult, LocalEnvironment) from adk-python - #582
Open
AmaadMartin wants to merge 6 commits into
Conversation
added 6 commits
July 30, 2026 11:08
…ironment Adds core/src/environment/, the adk-js counterpart of adk-python's src/google/adk/environment/: the abstract contract for "a place where an agent can run shell commands and read/write files", its result type, and the one concrete local implementation. BaseEnvironment and ExecutionResult are node-free so they ship from the browser-safe barrel; LocalEnvironment spawns child processes and is exported from the Node barrel only. Deliberately stricter than the reference: execute/readFile/writeFile guard on isInitialized rather than on the presence of a working directory, so they fail loudly before initialize() instead of silently working when a workingDir was supplied to the constructor.
…nd execute Unit tests for the ported environment abstraction: base-class lifecycle defaults and the initialization guard, plus LocalEnvironment's temporary vs caller-supplied workspaces, lexical path containment (including the sibling-prefix case a naive startsWith check would accept), file round-trips, and real child-process execution covering stdout, stderr, exit codes, cwd, env vars, timeout kill and spawn failure.
Follow-up from the simplicity review: drop the redundant "every field is required" sentence (the type already says so) and compress the exit-code mapping comment.
A shell that forks its command rather than exec'ing it (dash on ubuntu-latest, cmd.exe on windows-latest) leaves the command running after SIGKILL reaches the shell. The survivor keeps the stdio pipes open, so 'close' did not fire until it exited on its own: execute() blocked for the command's full runtime and the timeout was not enforced. Destroy the read ends alongside the kill. Also compare realpath on both sides of the cwd assertion; Windows CI reports the temp directory as an 8.3 short path.
A command killed by a timeout outlives the test, and Windows refuses to remove a directory that is a live process's cwd, so teardown failed with EBUSY. Use fs.rm's built-in retry, and document the same consequence for close() on the class.
… hook budget The 10s sleepers left a survivor that Windows kept the workspace locked on for the whole 10s, so the retrying afterEach blew vitest's 10s hook timeout. Halve the command lifetime, assert against a named bound rather than a bare number, and give the hook the same explicit budget as the spawning tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
Problem: adk-python has a small, self-contained abstraction for "a place where an agent can run shell commands and read/write files" (
src/google/adk/environment/). Everything file/shell related there is written against that interface, which is what lets the same tool run against a local subprocess or a remote sandbox. adk-js has no equivalent, so any future adk-js shell/file tool has to hardcode localchild_processaccess and can never be pointed at a sandbox.Solution: port the interface itself first, as an independent, cleanly landable change — the abstract contract, its result type, and the one concrete local implementation, nothing else.
core/src/environment/now exports:ExecutionResultenvironment/_base_environment.py(dataclass)BaseEnvironmentenvironment/_base_environment.pyLocalEnvironment/LocalEnvironmentOptionsenvironment/_local_environment.pyAll three are marked
@experimental.BaseEnvironmentandExecutionResultimport no Node built-ins, so they ship from the browser-safe barrel (core/src/common.ts);LocalEnvironmentspawns child processes and is exported from the Node barrel (core/src/index.ts) only, matching howUnsafeLocalCodeExecutoris wired.Deliberately out of scope (queued separately, not touched here): the
tools/environment/toolset, any Daytona/E2B sandbox implementation, and any refactor ofcore/src/code_executors/*ontoBaseEnvironment. Code executors are a different abstraction — they run a code snippet and returnCodeExecutionResult {stdout, stderr, outputFiles}, with no working-dir scoping, noreadFile/writeFileand no lifecycle — so the two result types are deliberately not unified. TheExecutionResultJSDoc says so explicitly to keep the two apart.Why one PR and not a stack. 823 added lines, but 481 of them are the two test files and the source is a single cohesive module. The only natural split —
BaseEnvironmentin part 1,LocalEnvironmentin part 2 — would make part 1 an exported abstract class with zero implementations and zero readers, which is worse to review, not better.Parity deviations (all deliberate)
is_initializedproperty with a setterisInitializedgetter only, backed byprotected initializedLocalEnvironmentalready does in Python (self._is_initialized = True).if self._working_dir is NoneassertInitialized()working_dirsets the attribute in__init__, soexecute/read_file/write_filework withoutinitialize()ever being called — and keep working afterclose(), which only clears_working_dirfor auto-created dirs. Here they fail loudly instead. Pinned by two tests.working_dir -> PathworkingDir: stringPathobject;stringis the adk-js convention (FileArtifactService,materializeFiles).execute(command, *, timeout)execute(command, timeoutSeconds?)UnsafeLocalCodeExecutorOptions.timeoutSeconds. No options bag for one optional parameter.read_file -> bytes,write_file(str | bytes)readFile -> Promise<Uint8Array>,writeFile(string | Uint8Array)Uint8Arrayrather thanBufferkeepsbase_environment.tsfree of Node types so it can live in the browser-safe barrel.Bufferis assignable toUint8Array, so no cast is needed.CodeExecutionResult, which makesstdout/stderr/outputFilesrequired though the Python original defaults them. Optional fields would push| undefinedonto every consumer. The default values still hold behaviourally and are pinned by a test asserting the exact zero-state object.Path.resolve()containment (resolves symlinks)path.resolve+path.relativecontainment (lexical only)fs.realpathwould fail for the not-yet-created file inwriteFile. The JSDoc says plainly that this is a lexical check and not a sandbox: it does not survive symlinks, hardlinks, bind mounts or TOCTOU.Parity rule applied where the two conflict: local TS convention wins for in-process concerns (naming,
private/protected, module layout); parity wins for anything observable — henceexitCodereproduces Python's negative signal number (-9for SIGKILL) rather than an invented sentinel, and the thrown message keeps Python'sPath escapes working directory:wording.Documented limitations (all stated in the JSDoc, all shared with the reference)
LocalEnvironmentruns arbitrary shell strings on the host with no sandboxing and no sanitisation; stdout/stderr are buffered fully in memory with no cap; the child inherits the whole ofprocess.env; and a timeoutSIGKILLs the shell, so processes it forked may survive. It is a building block — gating execution behind an explicit confirmation is the job of the (out-of-scope) tools built on top of it.Testing Plan
Unit Tests:
npx vitest run --project unit:core core/test/environment→ 31 passed (2 files), and green on all three CI legs (ubuntu-latest,macos-latest,windows-latest). Commands are built fromprocess.execPathwith outer double quotes so they run under bothshandcmd.exe; the cwd assertion normalisesfs.realpathon both sides, because macOS reachesos.tmpdir()through a symlink and Windows CI reports it as an 8.3 short path.Coverage of the new module (
--coverage.include='core/src/environment/**'):The single uncovered branch is the
?? 0fallback inexitCode: signal === null ? (code ?? 0) : -os.constants.signals[signal].It is unreachable by construction — Node guarantees exactly one of
code/signalis non-null on'close'— but TypeScript types both as nullable. Keeping it is the honest option: the alternative is a non-null assertion, and the fallback mirrors Python's ownproc.returncode or 0. No coverage pragma was added to hide it.A real defect the cross-platform CI caught
The first CI run was green on macOS and red on
ubuntu-latestandwindows-latest, both withexpected 10054 to be less than 9000—execute(cmd, 0.5)had blocked for the command's full 10s runtime. Root cause:spawn(…, {shell: true})runs the command under/bin/sh(bash on my workstation, dash on ubuntu-latest,cmd.exeon Windows). bashexecs a simple command, so the shell process is the command andSIGKILLreaches it; dash andcmd.exefork, so the kill only reaches the shell and the surviving command keeps the stdio pipes open — and'close'fires only once the process has exited and its stdio is closed. Reproduced locally by forcing the shell:Fix: destroy the read ends alongside the kill, so the timeout is enforced regardless of what the shell did with the command. Output produced before the kill is still returned. The surviving process itself is the documented limitation this port shares with adk-python; the JSDoc now also records its Windows consequence (a survivor keeps the working directory locked, so a
close()right after a timeout can fail to remove a temporary workspace). Actually killing the process tree —detached+ a process-group kill on POSIX,taskkill /Ton Windows — would remove the limitation entirely but goes beyond the reference implementation, so it is filed as follow-up work rather than smuggled into this port.A regression test pins this on every platform without depending on which shell is installed: it writes a script that spawns a child inheriting stdio, so the survivor exists even under bash.
The second, Windows-only failure was in the test's own teardown —
EBUSY: resource busy or locked, rmdir— the same survivor holding the workspace as its cwd. That was fixed in the tests only (fs.rmretries, shorter-lived commands, an explicit hook budget); no assertion was weakened to get CI green.Proof the tests can fail. Each mutation was applied to the finished implementation, the suite re-run, then reverted:
this.assertInitialized()fromexecute()rejects execute, readFile and writeFile before initialize()(+ the after-close()test)expected [Function] to throw error matching /not initialized/ but got 'spawn /bin/sh ENOENT'path.relativeguard withif (!resolved.startsWith(base))rejects a sibling directory that merely shares the name prefixexpected [Function] to throw error matching /escapes working directory/ but got 'ENOENT: no such file or directory…'timedOut = falsein the timeout handlerkills the command and reports timedOut once the timeout elapsesexpected false to be truethis.autoCreated &&fromclose()creates a caller-supplied workspace and keeps it after close()promise rejected "Error: ENOENT: no such file or directory…" instead of resolvingif (false)inBaseEnvironment.assertInitialized()rejects operations until a subclass marks it initializedpromise resolved "{ exitCode: +0, stdout: '', …(2) }" instead of rejectingchild.stdout.destroy()/child.stderr.destroy()from the timeout handlertimes out even when the command leaves a child holding the pipes openexpected 5296 to be less than 4000Manual End-to-End (E2E) Tests:
Run against the built package (
npm run build --workspace core), importing from@google/adk— no mocks, real child processes, real filesystem:Observed:
i.e. the temp directory appears under the OS temp dir while open and is gone after
close(), output round-trips,-9matches Python's SIGKILL return code,envVarsreach the child,readFile('../escape.txt')rejects, and a caller-suppliedworkingDirsurvivesclose().Other CI gates run locally on the rebased commit:
npm run build✅,npm run lint✅,scripts/check_license.sh✅,npx secretlint✅,npm run docs:check(typedoc--treatWarningsAsErrors) ✅, andtsc -p core --noEmitis clean.npm run format:checkreports no issue in any file this PR touches (its 16 remaining warnings are pre-existing onmain).npm run ts:checkat the repo root fails identically before and after this change (745 pre-existing errors on a clean tree, 745 with it, none in the new files).Simplicity review
An independent complexity audit returned "Lean. Faithful, tightly scoped port with no suppressions and no unrelated hunks. Nothing blocking." A second pass over the final diff, asked specifically to scrutinise the late CI fixes, returned "Lean already. Ship." with no findings. Two of the first pass's four nits were applied (trimmed a redundant JSDoc sentence and a comment). Three were declined, with reasons:
resolve(code ?? -1)instead of the negative signal number." Declined:exitCodeis an observable output of a ported API, so parity wins. Verified: the manual run returns-9for SIGKILL, matching Python.resolved !== base && !resolved.startsWith(base + path.sep)." Declined — it is 4 lines shorter but wrong on Windows, which CI runs.path.win32.relative('C:\Users\x\ws', 'c:\users\x\ws\a.txt')returns'a.txt'(case-folded, correctly accepted) whereas thestartsWithform returnsfalseand would reject a legitimate in-workspace path on a case-insensitive filesystem. Thepath.isAbsolute(relative)arm is likewise load-bearing:path.win32.relative('C:\ws', 'D:\evil.txt')returns'D:\evil.txt', which neither of the other two conditions catches. This also matches existing repo precedent incore/src/artifacts/file_artifact_service.ts.initialize()set the flag." Declined: Python's baseinitialize()is a pure no-op that leavesis_initializedfalse, and flipping the flag by default would let a subclass that failed to set itself up report as initialized — the opposite of the strictness this port exists to add.Checklist