Skip to content

Fix: detect PowerShell 7+ (pwsh) in the UnsafeLocalCodeExecutor SHELL branch - #255

Closed
AmaadMartin wants to merge 11 commits into
mainfrom
fix/shell-executor-pwsh-detection
Closed

Fix: detect PowerShell 7+ (pwsh) in the UnsafeLocalCodeExecutor SHELL branch#255
AmaadMartin wants to merge 11 commits into
mainfrom
fix/shell-executor-pwsh-detection

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Jul 29, 2026

Copy link
Copy Markdown
Owner

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

No issue was filed for this report.

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

Problem: CodeExecutionLanguage.SHELL ignores PowerShell 7+ (pwsh) when selecting spawn arguments and the script extension.

The SHELL branch of UnsafeLocalCodeExecutor.executeCode picked PowerShell-specific spawn arguments with a substring test against the literal string powershell:

if (this.shellCommandPath.toLowerCase().includes('powershell')) {
  args = ['-NoLogo', '-ExecutionPolicy', 'Bypass', '-File', filePath];
}

PowerShell 7+ ships as pwsh / pwsh.exe, not powershell — the executor's own CodeExecutionLanguage.POWERSHELL branch already acknowledges this (IS_WINDOWS ? 'powershell' : 'pwsh'). So new UnsafeLocalCodeExecutor({shellCommandPath: 'pwsh'}) produced an invocation that cannot work:

  • args stayed at the default [filePath], so none of -NoLogo, -ExecutionPolicy Bypass, -File were passed.
  • getExtensionForLanguage had the same blind spot, so on non-Windows hosts the script was written as script.sh. PowerShell refuses a -File argument without a .ps1 extension, so fixing the arguments alone would not have been enough.

Observed: spawn('pwsh', ['/tmp/.../script.sh']).
Expected: spawn('pwsh', ['-NoLogo', '-ExecutionPolicy', 'Bypass', '-File', '/tmp/.../script.ps1']) — the same shape the POWERSHELL language branch already produces.

The substring test had a second defect: /usr/local/powershell-helpers/run.sh was misclassified as PowerShell and received PowerShell flags.

Solution: Replace the substring test with a module-private predicate that matches on the executable name only:

function isPowerShellCommand(commandPath: string): boolean {
  return /^(powershell|pwsh)(\.exe)?$/i.test(path.win32.basename(commandPath));
}

and use it for both the spawn arguments and the script extension. path.win32.basename is used rather than path.basename because it splits on both / and \ on every platform, so a Windows-style path is handled correctly when the tests run on Linux/macOS CI. An exact-name allowlist is deliberate: a substring match is what caused this bug class in the first place, so /opt/pwsh-tools/bin/bash must not match.

Two intentional behavior changes, both fixes of clearly-wrong behavior:

  1. A shellCommandPath naming a PowerShell 7+ host now receives PowerShell flags and a .ps1 script instead of a bare positional .sh invocation. The previously produced invocation could not work.
  2. Commands that merely contain powershell as a substring but are not a PowerShell host (for example /usr/local/powershell-helpers/run.sh) no longer receive PowerShell flags. That match was accidental and produced a broken invocation.

Existing default behavior (bash off Windows, powershell on Windows, explicit cmd) is bit-for-bit identical. No public API, type, option, export, or dependency change.

Also in this diff, both small and load-bearing:

  • import {spawn} from 'child_process''node:child_process'. This was the only bare child_process specifier in the repository; every other file and this file's three sibling imports already use the node: prefix. It is also required for the test's module spy to intercept the same specifier the source imports.
  • createTempScriptFile and getExtensionForLanguage took shellCommandPath?: string, but the only call site passes this.shellCommandPath, which the constructor always defaults to a non-empty string. Both are now required, which retires an unreachable shellCommandPath && guard. Both functions are module-private, so this is not an API change.

Explicitly out of scope: cmd detection still uses includes('cmd') and is unchanged byte-for-byte, and -NoProfile is not added. Both are tracked separately; the new tests are written so that inserting -NoProfile later will not require rewriting them.

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.

The bug is about the argv handed to spawn, which the pre-existing tests could not observe (they assert on the stdout/stderr of real child processes). core/test/code_executors/unsafe_local_code_executor_test.ts now uses vitest's autospy — vi.mock('node:child_process', {spy: true}) — which wraps the real export without replacing its implementation, so all 17 pre-existing tests keep executing real child processes unchanged while the 8 new cases assert only on spawnSpy.mock.calls. That makes them deterministic on ubuntu-latest, windows-latest and macos-latest whether or not pwsh/cmd exists on the runner: a missing binary just resolves through the existing Process error: path after the arguments were already recorded.

New describe('shell command detection') cases:

  • PowerShell is detected for pwsh, pwsh.exe, /usr/bin/pwsh, C:\Program Files\PowerShell\7\pwsh.exe, PWSH, and (regression) powershell, powershell.exe — each gets the PowerShell flags, -File immediately before the script path, and a script.ps1 extension.
  • No substring misfire for /opt/pwsh-tools/bin/bash and /usr/local/powershell-helpers/run.sh — argv is the bare [filePath].
  • cmd and cmd.exe are unaffected — argv is ['/c', <script>].

Commands run locally on the pushed commit:

npx vitest run --project unit:core core/test/code_executors/unsafe_local_code_executor_test.ts   # 25 passed
npx vitest run --project unit:core core/test/code_executors core/test/tools/skills               # 193 passed
npm run build        # OK
npm run lint         # OK
npm run format:check # OK
npm run docs:check   # OK (typedoc --treatWarningsAsErrors)
npx secretlint       # OK

The new tests were verified to actually catch the bug: reverting only core/src/code_executors/unsafe_local_code_executor.ts to its pre-fix state fails 8 of them (all 7 PowerShell rows plus the /usr/local/powershell-helpers/run.sh misfire row) and leaves the other 17 passing.

The shell command detection block carries an explicit 30s timeout. CI runners that ship PowerShell really launch it for these cases, and the first launch on a cold runner exceeded vitest's default 5s timeout — which is itself evidence the fixed invocation is accepted by a real PowerShell host. run-tests passes on ubuntu-latest, windows-latest and macos-latest.

Manual End-to-End (E2E) Tests:

Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

On a host with PowerShell 7 installed:

import {CodeExecutionLanguage, UnsafeLocalCodeExecutor} from '@google/adk';

const executor = new UnsafeLocalCodeExecutor({shellCommandPath: 'pwsh'});
const result = await executor.executeCode({
  invocationContext,
  codeExecutionInput: {
    code: 'Write-Host "hello from pwsh"',
    language: CodeExecutionLanguage.SHELL,
    inputFiles: [],
  },
});

result.stdout contains hello from pwsh and result.stderr is empty. Before this change the script was written as .sh and handed to pwsh as a bare positional argument, so nothing was executed.

PowerShell 7 was not installed on the machine used for development, so this was verified end-to-end against the built package with no mocks by putting an executable named pwsh on disk that enforces the PowerShell -File contract (it exits 64 unless it receives exactly -NoLogo -ExecutionPolicy Bypass -File <path>.ps1, then runs the script). Real process spawn, real temp-file I/O:

RESULT /tmp/.../bin/pwsh  stdout="hello from /tmp/.../bin/pwsh\n"  stderr=""
RESULT bash               stdout="hello from bash\n"               stderr=""

Against the pre-fix build the same run fails with pwsh: expected '-NoLogo -ExecutionPolicy Bypass -File <script>', got: /tmp/.../script.sh. The bash row confirms the default path is unchanged.

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.

AmaadMartin and others added 11 commits July 28, 2026 14:48
* docs: document minimum supported Node.js version in README

Add a short prerequisite note under the Installation section stating that
ADK for TypeScript requires Node.js 18 or newer, so new users know which
Node.js runtime they need before running npm install @google/adk.

The version reflects the mandated fallback: no engines.node field is
declared in any package.json in the repo.

* docs: reference current Node.js LTS instead of a fixed version

Node.js 18 is EOL and any hard-coded minimum version goes stale over time.
Reword the installation prerequisite to point readers at the current Node.js
LTS releases, which stays accurate without future edits.

Addresses PR review feedback on #526.

---------

Co-authored-by: Amaad Martin <amaadmartin@google.com>
…#536)

`ToolAuthHandler` accepted an `authCredential` and then never read it. When
no auth response was present it went straight to `requestCredential()`, so a
credential handed to `OpenAPIToolset`/`RestApiTool` at construction time was
ignored and the tool returned `{pending: true}` on every call. For `apiKey`,
`http` and `serviceAccount` schemes nothing ever resolves that request — no
user interaction is involved — so the tool could never complete.

Fall back to the configured credential when there is no auth response, which
mirrors `_get_auth_response() or self.auth_credential` in adk-python.

Also narrow what gets written to session state. The credential store exists
to avoid repeating work that either cannot be repeated (an auth response is
readable once) or is expensive (an exchange costs a round trip). A static
credential that needed no exchange is neither, so it is no longer persisted —
that would only copy the developer's secret into the session store.
…hon parity (#542)

* Feat: add LoadMcpResourceTool and MCPToolset resource access

Port adk-python's LoadMcpResourceTool to adk-js for cross-language parity.

- Add listResources/getResourceInfo/readResource to MCPToolset, following
  the existing create -> try -> closeSession-in-finally session idiom.
- Add LoadMcpResourceTool (mirrors the in-repo LoadArtifactsTool idiom):
  declares load_mcp_resource({resource_names}), and processLlmRequest injects
  resolved resource contents (text + base64 binary, no decode step) into the
  LlmRequest.
- Export the tool from core/src/index.ts (@google/adk public API).

* test: cover LoadMcpResourceTool and MCPToolset resource access

Add full unit coverage (100% line + branch of the new code):

- load_mcp_resource_tool_test.ts: init, declaration, runAsync (incl. default),
  list injection (incl. empty + swallowed list errors), text/binary/unknown
  content, base64 blob passthrough + default mime type, swallowed read errors,
  and all no-op guard paths (non-matching/absent function response, missing
  parts).
- mcp_toolset_test.ts: listResources/getResourceInfo/readResource happy paths
  and error paths (unknown name, missing URI), plus session-cleanup assertions
  for success and failure (closeSession in finally, no leaked sessions).

* test(e2e): exercise LoadMcpResourceTool against a real MCP server

Add a no-mock end-to-end test that spawns a real MCP server over stdio
(mcp_resource_server.mjs, exposing a text and a binary resource) and drives
the real MCPToolset + LoadMcpResourceTool: listing/resolving/reading resources
and injecting their contents (text + base64 binary) into an LlmRequest.

---------

Co-authored-by: Amaad Martin <amaadmartin@google.com>
* feat(tools): add ExampleTool for few-shot examples

Port adk-python's ExampleTool to adk-js. The tool accepts a static
Example[] or a BaseExampleProvider and, on each outgoing LLM request,
appends a few-shot <EXAMPLES> block (built via buildExampleSi from the
latest user query) to the system instruction. It is never declared to
the model (mirrors PreloadMemoryTool) and is a no-op when no user text
is present. Exported from the public @google/adk API.

* test(tools): cover ExampleTool unit and end-to-end paths

Add Vitest coverage for ExampleTool: static list and provider paths,
model-style passthrough, no-op branches (missing user content, empty
parts, text-less first part), runAsync throwing, and the public export.
Includes an end-to-end block that drives processLlmRequest through a
real Context/InvocationContext (no mocks). 100% line/branch coverage of
the new tool.

* refactor(tools): apply simplicity audit feedback

Use a constructor parameter property for `examples` (repo convention),
and drop the redundant provider end-to-end test whose only unique aspect
was a spy — keeping the no-mock e2e block strictly mock-free. The
provider selection path stays fully covered by the unit tests; the tool
retains 100% line/branch coverage.

---------

Co-authored-by: Amaad Martin <amaadmartin@google.com>
* feat(agents): support clone() for RoutedAgent

RoutedAgent derives its routing targets from config.agents rather than
subAgents, so the inherited BaseAgent.clone() rebuilt the agent from the
already-parented originals and threw "already has a parent agent".

Add a RoutedAgent.clone() override that deep-clones the routing targets
(via a private cloneRoutingTargets helper) and passes them through the
agents override, so super.clone() rebuilds the constructor with fresh,
detached copies that are re-parented onto the clone. The array-vs-record
shape and record keys are preserved so the clone routes identically, and
parent-override rejection plus the detached-root guarantee are still
enforced by the base implementation.

Remove the now-obsolete "documented limitation" test (and its unused
RoutedAgent import) from base_agent_test; positive coverage lives in
routed_agent_test.

* test(agents): cover RoutedAgent.clone()

Add a clone describe suite exercising the new override and the
cloneRoutingTargets helper: array and record forms, deep-clone and
re-parenting of targets, originals left untouched, functional routing on
the clone (record form), verbatim agents override, non-agents overrides,
and parentAgent-override rejection. Includes a no-mock end-to-end case
that clones a RoutedAgent whose targets are real LlmAgents.

---------

Co-authored-by: Amaad Martin <amaadmartin@google.com>
* Feat(tools): add SSRF-safe load_web_page tool for adk-python parity

Ports the adk-python load_web_page tool to adk-js. Fetches a URL and
returns its extracted, readable text, hardened against SSRF:

- only http/https schemes are fetched
- localhost-style hostnames and hosts resolving to non-global IPs
  (private, loopback, link-local, shared/CGNAT, reserved, multicast,
  IPv4-mapped IPv6) are rejected before any connection
- redirects are never followed (redirect: 'manual')
- a configurable timeout (default 30s) bounds every request
- expected failures return the parity string "Failed to fetch url: <url>"
  instead of throwing

Exposes loadWebPage(), the LOAD_WEB_PAGE FunctionTool, and the
LoadWebPageOptions type via the @google/adk public API.

* Refactor(tools): inline single-use failure prefix in load_web_page

Addresses simplicity-audit feedback: the FAILURE_PREFIX constant had a
single caller, so its literal is inlined into failedToFetchMessage, which
remains the sole formatter of the parity failure string.

---------

Co-authored-by: Amaad Martin <amaadmartin@google.com>
The SHELL branch of UnsafeLocalCodeExecutor selected PowerShell spawn
arguments with a substring test against `powershell`, so PowerShell 7+
(`pwsh`) was invoked without `-NoLogo -ExecutionPolicy Bypass -File` and
its script was written with a `.sh` extension, which PowerShell refuses
to run. The same substring test also misclassified unrelated commands
whose path merely contains `powershell`.

Detect PowerShell hosts on the executable name only (`powershell`/`pwsh`,
case-insensitive, with or without `.exe`, either path separator) and use
that for both the spawn arguments and the script extension.
Use path.win32.basename instead of a hand-rolled separator split (it
splits on both separators on every platform), drop the two-element Set
in favour of a direct comparison, and derive the spawn passthrough types
in the test from the real spawn signature.
Collapse the name check into a single anchored regex and drop assertions
that restate behaviour already covered elsewhere in the file.
Replace the hand-written passthrough mock factory with
vi.mock(..., {spy: true}), which wraps the real export without replacing
its implementation, and pin -File to the argument before the script path.
CI runners that ship PowerShell really launch it for these cases, and the
first launch on a cold runner exceeded the default 5s test timeout.
@AmaadMartin
AmaadMartin force-pushed the fix/shell-executor-pwsh-detection branch from a0b2373 to 2f6ba68 Compare July 29, 2026 18:05
@AmaadMartin

Copy link
Copy Markdown
Owner Author

Automated: ported to upstream as google#568.

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.

2 participants