Test: drop the unresolvable @google/adk deep import from the sandbox executor test (stacked on #486) - #632
Open
AmaadMartin wants to merge 2 commits into
Conversation
added 2 commits
August 4, 2026 05:08
…executor test The sandbox integration test imported the code-execution response processor via '@google/adk/agents/processors/code_execution_request_processor.js'. core/package.json declares only "." in its exports map, so that specifier is not a resolvable subpath of the published package -- it only resolved locally because vitest.config.ts aliases @google/adk to ./core/src as a prefix. tsc reports it as TS2307 "Cannot find module". Now that the processor is installed by default, no test needs to reach across the package boundary to get it. Drop the import and retarget the test that was its only consumer: instead of hand-installing the processor (which the new default-wiring case already covers with strictly stronger assertions), it now pins the complementary override branch -- a caller-supplied responseProcessors array replaces the default, so no code is executed.
The previous commit retargeted the sandbox test that used to pin "an explicit responseProcessors list containing the code-execution processor still executes code". The default-wiring case does not subsume that branch, so it was left untested. Restore it as its own case in the unit test, where importing the non-exported singleton relatively is the established in-package pattern and does not need an unresolvable @google/adk subpath. The explicit list is read back from a default-constructed agent rather than built from the relatively-imported singleton directly. That keeps the value and the LlmAgentConfig type on the same side of the core/src vs core/dist boundary (mixing them is a TS2322), and it mirrors the only route actually available to an external caller, since the singleton is not part of the public API. Shared setup and assertions for the two end-to-end cases are hoisted into runCodeExecutionAgent / expectCodeWasExecuted; assertions are unchanged in substance. In the sandbox test, the mock client and executor are now built by one createSandboxFixture helper, which drops one of the two boundary casts.
7 tasks
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
No public GitHub issue was supplied with this task.
Stacked on #486 (
--base fix/code-execution-response-processor-default). This PR is the residual cleanup that #486 leaves behind; it is deliberately not a competing implementation of the auto-wiring.Collision check (performed before any code was written)
531 open PRs scanned. Four are adjacent to the code-execution response processor:
CODE_EXECUTION_RESPONSE_PROCESSORinto theLlmAgentdefaultscore/src/common.tsThe assigned task was "auto-wire the code-execution response processor into
LlmAgent". #486 implements that in full, so building a fourth implementation would have been duplicate work colliding on the same lines. What #486 does not do is satisfy the task's final postcondition — no@google/adk/...deep subpath specifier anywhere undertests/. This PR closes exactly that gap and nothing else.Problem:
tests/integration/agents/agent_with_sandbox_executor_test.tsimports the response processor across the package boundary:core/package.jsondeclares only"."in itsexportsmap, so this is not a resolvable subpath of the published package. It only resolves in-repo becausevitest.config.tsaliases@google/adkto./core/srcas a prefix alias. This is not merely stylistic — it is a real type error thattscalready reports today:(
npm run ts:checkis not one of the CI steps in.github/workflows/validation.yaml, which is why this has survived.)Solution: Now that #486 installs the processor by default, no test needs to reach across the package boundary to obtain it. Drop the import, and retarget the one test that consumed it.
That test (
executes code generated by the agent) hand-installed the processor viaresponseProcessors: [CODE_EXECUTION_RESPONSE_PROCESSOR]. Simply deleting that property would have made it a byte-for-byte duplicate of theexecutes code with no explicit responseProcessorscase #486 added. Instead it now pins the complementary branch ofconfig.responseProcessors ?? [...]: a caller-supplied array replaces the default, so no code is executed. The two tests together now cover both sides of that??, where before neither test distinguished them.On the coverage the retarget displaced. The old assertions were
events.length >= 3andhasExecutionResult(any part whosetextcontains'hello'). The "code executes" half of that is subsumed, strictly more strongly, by the survivingexecutes code with no explicit responseProcessorscase, which assertsexecuteCodeInternalwas called exactly once,resultPartshas length 1, outcomeOutcome.OUTCOME_OK, text containing'hello', and final event text'Execution was successful.'. The oldhasExecutionResultpredicate was in fact too weak to be a regression signal at all: the model's first mocked response text already containsprint("hello"), so it passed even with the response processor absent.Subsumption did not hold for the other thing that test pinned, however: an explicit, non-empty
responseProcessorslist containing the code-execution processor still executes code. That is a distinct branch ofconfig.responseProcessors ?? [...]— and it is the branch whose semantics this stack changes. It is restored as its own case incore/test/agents/llm_agent_test.ts:All four branches are now covered: default with executor, default without executor, explicit empty list, explicit non-empty list.
It is restored in the unit test rather than the integration test deliberately: reconstructing it under
tests/integration/would require re-adding the very@google/adkdeep import this PR exists to remove. Incore/test/, importing the non-exported singleton via a relative../../src/...path is the established in-package pattern and resolves correctly.One subtlety worth flagging, because it constrains how the test is written. The explicit list is read back from a default-constructed agent (
new LlmAgent({name: 'probe_agent'}).responseProcessors) rather than written literally as[CODE_EXECUTION_RESPONSE_PROCESSOR]. Passing the relatively-imported singleton straight into anLlmAgentConfigtyped through@google/adkmixes thecore/srcandcore/dist/typescopies ofBaseLlmResponseProcessorand is a hardTS2322. Reading the array back keeps both sides on one side of that boundary — and it mirrors the only route actually available to an external caller, since the singleton is not public API. The test assertsexpect(explicitProcessors).toContain(CODE_EXECUTION_RESPONSE_PROCESSOR)first, so it cannot silently degenerate into the default case.Shared setup and assertions for the two end-to-end unit cases are hoisted into
runCodeExecutionAgent/expectCodeWasExecuted; the assertions are unchanged in substance. In the sandbox test,createSandboxFixture()now builds the mock client and executor together, reducing theas unknown as Clientboundary casts from two to one.Design decision (inherited from #486, restated for the reviewer)
Auto-wire the processor; keep the singleton internal. Justified by
adk-python, wheresrc/google/adk/flows/llm_flows/single_flow.pyinstalls_code_execution.request_processor(line 68) and_code_execution.response_processor(line 80) by default, and the module is_code_execution— underscore-prefixed, i.e. deliberately private. Users get it by construction, never by import. This PR is consistent with that: it removes the only import site rather than adding a public export.This is the point on which #460 and #486 disagree. #460 removes the same deep specifier by exporting the singleton from
core/src/common.ts, which makes theadk-jspublic surface strictly larger than Python's and is asymmetric withCODE_EXECUTION_REQUEST_PROCESSOR, which is likewise unexported. #460 and #486 cannot both merge as written — that is a maintainer call, flagged here rather than pre-empted.Behavior change note (from the stack as a whole)
Setting
codeExecutoron anLlmAgentis now sufficient to execute model-written fenced code blocks; previously the feature was inert for every non-built-in executor. Two consequences worth calling out:codeExecutorpurely to back therun_skill_script_toolfallback (core/src/tools/skill/run_skill_script_tool.ts) will now also execute fenced code blocks the model writes in free text. That is the intended meaning ofcodeExecutorand matchesadk-python.postProcessCodeExecutionResultincore/src/agents/processors/code_execution_request_processor.tsthrows'Artifact service is not initialized.'wheninvocationContext.artifactServiceis undefined. This only fires on a configuration that was already non-functional, andInMemoryRunner/Runnersupply an artifact service. Behavior deliberately left unchanged.BuiltInCodeExecutorandrunConfig.supportCfcruns are unaffected — the response processor returns early for built-in executors.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.
Proof the tests can fail (mutation testing). Two mutations were applied to
core/src/agents/llm_agent.ts, each caught by the complementary test — neither test passes vacuously.Mutation A — swap nullish coalescing for a truthy-length check (the realistic bug this PR's retargeted test guards against, where an explicitly-supplied empty array is wrongly treated as "not provided"):
Mutation B — revert the default to
[](i.e. undo #486's fix entirely):Under Mutation B the retargeted override test correctly still passes — it pins the other branch.
Mutation C — discard any caller-supplied list (the mutation that isolates the restored explicit-non-empty branch):
The other three branch tests pass under Mutation C, confirming the restored case carries signal none of them do. (The pre-existing abort test also supplies a custom list, so it is legitimately caught too.)
Source was restored from a pristine copy after each mutation and re-verified green (42 passed).
Type-error evidence. Diffing
tsc --noEmit --pretty falseoutput before and after this change, the single error removed is exactly the deep import:The remaining 280 are pre-existing and identical on the base — they are
core/dist/typesvscore/srcduplicate-declaration conflicts produced by runningtsc --noEmitafternpm run buildin one checkout, in files this PR does not touch.ts:checkis not a CI step.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
CI note:
ci_status = absent..github/workflows/validation.yamltriggers onpull_request: branches: [main], and this PR targetsfix/code-execution-response-processor-default, so therun-testsjob will not fire. Every CI gate was therefore run locally on the exact pushed commit:To walk the loop by hand, run the sandbox integration test above: the mocked model emits a fenced
```pythonblock, the executor'sexecuteCodeInternalis invoked once, acodeExecutionResultpart withOutcome.OUTCOME_OKis emitted, and the model then summarises. No credentials or live model are required.Verification that the postcondition now holds:
(Two deep specifiers remain under
core/test/sessions/vertex_ai_session_service_test.ts; those are outside this postcondition's scope and are already addressed by #512.)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.