Skip to content

Fix: export CODE_EXECUTION_RESPONSE_PROCESSOR and cover the default code-execution path (stacked on #486) - #561

Open
AmaadMartin wants to merge 4 commits into
fix/code-execution-response-processor-defaultfrom
fix/default-code-execution-response-processor
Open

Fix: export CODE_EXECUTION_RESPONSE_PROCESSOR and cover the default code-execution path (stacked on #486)#561
AmaadMartin wants to merge 4 commits into
fix/code-execution-response-processor-defaultfrom
fix/default-code-execution-response-processor

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 3, 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):
    Closes: #issue_number
    Related: #issue_number
  2. Or, if no issue exists, describe the change:

Stacked on #486 (fix/code-execution-response-processor-default). Review that one first; this PR's own delta is the last four commits (09b17b11, 9e62a4ec, e4886371, 8ed5d7fd) across 6 files.

Collision check (run before any code was written).
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 returned 460 open PRs; filtering for code[_ -]?exec|responseProcessor|processor|llm_?agent|planner surfaced 31 candidates, of which three were inspected with gh pr diff --name-only:

Problem: #486 makes CODE_EXECUTION_RESPONSE_PROCESSOR the default response processor, but the symbol is still unreachable from an installed @google/adk. It is absent from core/src/common.ts, and core/package.json declares only a "." entry in its exports map, so there is no deep subpath either. Two consequences:

  1. A caller who supplies their own responseProcessors list replaces the default (that is the same contract requestProcessors has), and so silently loses code execution — with no way to add it back, because the symbol cannot be imported.
  2. tests/integration/agents/agent_with_sandbox_executor_test.ts imports it from @google/adk/agents/processors/code_execution_request_processor.js, a path that resolves only because vitest.config.ts aliases @google/adk to ./core/src. It is not reachable for a real consumer, so the test was proving something users cannot do.

Solution: export the singleton and its class from common.ts, document the replace-not-merge contract, and add the integration coverage #486 left out.

  1. core/src/common.ts — export CODE_EXECUTION_RESPONSE_PROCESSOR and CodeExecutionResponseProcessor, sorted by module specifier between the base_llm_processor.js and content_request_processor.js blocks. index.ts and index_web.ts both re-export common.js, so this single edit covers the node entry point, the web entry point, and TypeDoc. Both symbols are required: the class appears in the public type of the const, so exporting only the const fails docs:check. No new subpath was added to the exports map, and CODE_EXECUTION_REQUEST_PROCESSOR was deliberately not exported — it is not public today and nothing here needs it.
  2. core/src/agents/llm_agent.ts — JSDoc only. States that omitting responseProcessors selects the default and that supplying a list replaces it entirely, so a caller needing code execution alongside their own processors must include the default explicitly. This is the contract that made the gap above a real trap.
  3. Tests import through the public entry point. llm_agent_test.ts moves its CODE_EXECUTION_RESPONSE_PROCESSOR import from a relative ../../src/... path to @google/adk, and the sandbox test drops the deep subpath entirely. This pins the public export: mutation C below shows both fail without the common.ts change. code_execution_request_processor_test.ts is deliberately left untouched — an earlier revision changed it too, but nothing required that, and it left one module reached through two different specifiers.
  4. New tests/integration/agents/agent_with_default_code_execution_test.ts — the default-path coverage. Uses a stub BaseCodeExecutor rather than UnsafeLocalCodeExecutor so the test does not depend on python3 on the CI host.

A fixture-isolation bug this surfaced, and fixed in both places. The negative control initially failed with expected 'Here is the code:\n' to contain '```python...'. That is not a product bug. extractCodeAndTruncateContent rewrites content.parts in place (code_execution_utils.ts:182-188), createLlmResponse passes content: candidate.content by reference (llm_response.ts:133), and GeminiWithMockResponses assigns the caller's Candidate objects straight through (test_case_utils.ts:53). A fixture held in a module-level const is therefore rewritten by whichever test runs first.

This affected the sandbox test too, which is why the fix is applied there as well rather than deferred: its two cases shared one MOCK_RESPONSES, so the first permanently rewrote the second's input and the second silently exercised the extractor's executableCode branch instead of the markdown-code-block branch it was written to cover. It still passed, which is worse than failing — the result flips with run order. Both files now build their fixture from a mockResponses() factory.

Demonstrated directly with a throwaway probe (not committed) that snapshots the fixture around a single run:

Expected: [{"text":"Here is:\n```python\nprint(\"hello\")\n```"}]
Received: [{"text":"Here is:\n"},{"text":"print(\"hello\")","executableCode":{"code":"print(\"hello\")","language":"PYTHON"}}]

Two related cleanups in the same file, both net reductions: the two call-site mockClient as unknown as Client casts collapse into the mock factory, which now returns the already-typed client alongside the execution spy (1 cast total, same as main before this stack, down from 2); and the duplicated event-draining loop moves to collectEvents in tests/integration/test_case_utils.ts, used at all five call sites. The file also loses main's eslint-disable @typescript-eslint/no-explicit-any and any[].

Known, accepted behavior change (inherited from #486, restated because it is easy to miss). postProcessCodeExecutionResult() throws Error('Artifact service is not initialized.') when invocationContext.artifactService is undefined. An agent with a codeExecutor on a Runner built without an artifact service now reaches that throw where it previously did nothing. This is intentional parity — adk-python raises from the identical guard in _code_execution.py and registers the processor unconditionally. InMemoryRunner always supplies an InMemoryArtifactService, so the default setup is unaffected. No silent fallback was added.

Parity note. Where local convention and parity conflict, parity wins for observable behavior and local convention wins for naming: the default list and its ordering match SingleFlow._create_response_processors(), while the symbol is named CODE_EXECUTION_RESPONSE_PROCESSOR to match the SCREAMING_SNAKE_CASE singleton convention in core/src/agents/processors/ rather than Python's response_processor.

No suppressions. No any, as any, @ts-expect-error, @ts-ignore, or eslint-disable anywhere in this diff (verified by grep over git diff, output empty).

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:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

npx vitest run --project unit:core core/test/agents/llm_agent_test.ts \
  core/test/agents/processors/code_execution_request_processor_test.ts
  -> Test Files 2 passed (2) | Tests 39 passed (39)

npx vitest run --project integration \
  tests/integration/agents/agent_with_default_code_execution_test.ts \
  tests/integration/agents/agent_with_sandbox_executor_test.ts
  -> Test Files 2 passed (2) | Tests 5 passed (5)

npx vitest run --project integration tests/integration/agents   # whole dir, shared helper
  -> Test Files 4 passed (4) | Tests 12 passed (12)

CI status: absent — validated locally instead. .github/workflows/validation.yaml triggers on pull_request: branches: [main]. This PR is stacked, so its base is fix/code-execution-response-processor-default and the workflow never fires; the only check that ran is auto-assign, which is not validation. Every step validation.yaml would have run was therefore run locally against the exact pushed commit 8ed5d7fd, and all pass:

npx secretlint "**/*"   -> clean (no findings)
npm run build           -> OK
npm run lint            -> OK
npm run format:check    -> OK
npm run docs:check      -> OK

Repo gates, on the exact commit pushed (8ed5d7fd):

npm run lint          -> clean (no output)
npm run format:check  -> "All matched files use Prettier code style!"
npm run docs:check    -> clean (typedoc --emit none --treatWarningsAsErrors)
npm run build         -> succeeds
npm run ts:check      -> 280 errors, all pre-existing.
                         Baseline at the stacked base 675d97b3 is 281, so this
                         change removes 1 and adds none.

ts:check fails repo-wide on main today, so per-file counts are the honest measure. Comparing 675d97b3 to 8ed5d7fd, exactly one file changes and it goes down; no file goes up:

tests/integration/agents/agent_with_sandbox_executor_test.ts:  1 -> 0

The repo's remaining failures are a pre-existing dual-resolution problem (tsconfig resolves @google/adk to core/dist/types while relative imports resolve to core/src, so the same class is two nominal types — "Types have separate declarations of a private property"). That affects ~40 test files and is out of scope here.


**Proof the tests can fail.** Every new test was run against mutated source and confirmed to FAIL:

| #   | Mutation                                                                                                                            | Result                                                                                                                                                                                                                                                                                   |
| --- | ----------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| A   | `llm_agent.ts`: revert the default to `config.responseProcessors ?? []`                                                             | **6 killed.** Unit: `expected [] to include CodeExecutionResponseProcessor{}` (x2) and `expected [] to have a length of 1 but got +0`. Integration: both new cases plus the sandbox test's no-explicit-processors case. The negative control correctly still passes — it asserts inertness. |
| B   | `llm_agent.ts`: default to `[new CodeExecutionResponseProcessor()]` instead of the shared singleton                                 | **2 killed.** `expected [ CodeExecutionResponseProcessor{} ] to include CodeExecutionResponseProcessor{}` — fails on identity, pinning the singleton invariant.                                                                                                                          |
| C   | `common.ts`: remove the new export block (this PR's own change)                                                                     | **3 killed.** The 2 `llm_agent_test` default cases and the sandbox integration test (`expected 1 to be greater than or equal to 3`). Confirms the public export is load-bearing, not decorative. |
| D   | `code_execution_request_processor.ts:313`: weaken the no-executor guard to `if (codeExecutor && !isBaseCodeExecutor(codeExecutor))` | **1 killed.** Negative control: `expected '' to contain '```python\nprint("hello")\n```'`.                                                                                                                                                                                               |
| E   | `code_execution_utils.ts:218`: make the `stderr` branch unreachable                                                                 | **1 killed.** Error-path test: `expected 'OUTCOME_OK' to be 'OUTCOME_FAILED'`.                                                                                                                                                                                                           |

Manual End-to-End (E2E) Tests:
A real e2e run needs live model credentials, and the sandbox executor additionally needs a Vertex project, so no `tests/e2e` case is added. The two integration tests are the stand-in: both drive a real `InMemoryRunner` and the real processor pipeline end to end, with only the model and the executor stubbed. To reproduce the user-visible fix by hand:

```ts
const agent = new LlmAgent({
  model, // any real model
  name: 'coder',
  codeExecutor: new UnsafeLocalCodeExecutor(), // no responseProcessors
});
const runner = new InMemoryRunner({agent});
for await (const e of runner.runAsync(/* ... */)) console.dir(e, {depth: null});

Ask it to print something. Before #486 the only events are the user message and a model text event containing the raw ```python block. With this stack you additionally get an execution-result event whose part has codeExecutionResult.outcome === Outcome.OUTCOME_OK and text starting Code execution result:. Requires python3 on PATH for UnsafeLocalCodeExecutor.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 4 commits August 2, 2026 21:32
The LlmAgent default (stacked base) makes CODE_EXECUTION_RESPONSE_PROCESSOR
the out-of-the-box response processor, but the symbol was still unreachable
from an installed @google/adk: it was absent from common.ts and core's
exports map declares no deep subpath. Callers who supply their own
responseProcessors list therefore could not re-add the default, and the
sandbox integration test could only import it through a path that resolves
via the vitest alias.

Export both the singleton and its class from common.ts (index.ts and
index_web.ts re-export it), document the replace-not-merge contract on
LlmAgentConfig.responseProcessors, and switch the tests to the public import.
The negative control read events.at(-1)?...?.text, which is undefined when a
regression truncates the turn, so the assertion failed with a type complaint
instead of a readable diff. Join the text across all parts so the mutation
'remove the codeExecutor guard in runPostProcessor' reports
"expected '' to contain '```python...'".
The sandbox test's module-level MOCK_RESPONSES was mutable shared state:
extractCodeAndTruncateContent rewrites content.parts in place and
GeminiWithMockResponses passes the caller's Candidate objects straight
through, so the first test permanently rewrote the second's input from a
markdown code block into an executableCode part. The second test still
passed, but via a different extractor branch than the one it was written
to cover, and the result flipped with run order. Build the fixture from a
factory so each test gets its own.

Also fold the two call-site 'as unknown as Client' casts into the mock
factory, which now returns the typed client alongside the execution spy,
and move the duplicated event-draining loop into test_case_utils.

Revert the code_execution_request_processor_test.ts import change: nothing
required it, and it left one module reached through two specifiers. The
public export stays pinned by llm_agent_test.ts and the sandbox test.
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