Fix: honour ADK_DISABLE_GEMINI_MODEL_ID_CHECK in BuiltInCodeExecutor and UrlContextTool - #470
Open
AmaadMartin wants to merge 4 commits into
Open
Fix: honour ADK_DISABLE_GEMINI_MODEL_ID_CHECK in BuiltInCodeExecutor and UrlContextTool#470AmaadMartin wants to merge 4 commits into
AmaadMartin wants to merge 4 commits into
Conversation
added 4 commits
August 1, 2026 10:36
The model-id escape hatch is honoured by vertex_ai_search_tool, enterprise_web_search_tool and google_maps_grounding_tool, but the built-in code executor still hard-throws for any non-Gemini-2+ model id. Consult isGeminiModelIdCheckDisabled() in the gate so a user who opts out of the naming convention gets a consistent bypass, matching adk-python's built_in_code_executor.process_llm_request. The llmRequest.model guard is retained: a request that names no model still throws rather than attaching a code-execution tool.
Both model guards now respect the escape hatch: a non-Gemini id such as internal-model-v1 fails isGeminiModel and isGemini2OrAbove, so bypassing only the first would leave the second throwing and the flag still broken. The bypass excludes Gemini 1.x, which does not support url_context at all. adk-js folds Python's dedicated Gemini-1.x rejection into the !isGemini2OrAbove guard, so a bare disjunction would let gemini-1.5-pro through with the flag set - something adk-python's url_context_tool rejects before the bypass is even considered. google_maps_grounding_tool encodes the same shape.
Save-and-restore of the original value restores the ambient value, so a developer with ADK_DISABLE_GEMINI_MODEL_ID_CHECK exported in their shell would see the two pre-existing 'throws for an unsupported model' cases fail - the source now honours the flag, so those assertions are only valid with it off. Stub the variable to undefined before each case (vi.unstubAllEnvs restores the ambient value at teardown) so both suites are green whether or not the flag is set in the environment.
UrlContextTool.processLlmRequest destructures {llmRequest} only, so the
Context -> InvocationContext -> LlmAgent/createSession/PluginManager graph
populated a parameter the code under test never reads, while coupling a
model-id gating test to three unrelated constructors.
Pass toolContext: {} as never like the six pre-existing cases in this file and
the analogous cases in the enterprise_web_search and google_maps_grounding
suites. The cast is inert here and its removal is being handled file-wide as a
separate cleanup.
This was referenced Aug 4, 2026
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
Closes: #issue_number
Related: #issue_number
Problem:
ADK_DISABLE_GEMINI_MODEL_ID_CHECKis a documented opt-in escape hatch for users whose model ids do not follow the publicgemini-*naming convention (custom endpoints, internal aliases). In adk-js it is only half-wired:isGeminiModelIdCheckDisabled()(core/src/utils/model_name.ts) is consulted byvertex_ai_search_tool.ts,enterprise_web_search_tool.tsandgoogle_maps_grounding_tool.ts, but two call sites that gate on the same concept never read it and hard-throw instead:core/src/code_executors/built_in_code_executor.ts—Gemini code execution tool is not supported for model internal-model-v1core/src/tools/url_context_tool.ts—URL context tool is not supported for model internal-model-v1With the flag set,
ENTERPRISE_WEB_SEARCH.processLlmRequest(...)succeeds on the same request while those two throw. adk-python has the bypass in both places (code_executors/built_in_code_executor.py:46-47,tools/url_context_tool.py:51-67).Solution: read the existing helper at both gates. No new types, exports, config fields, or public API;
process.envis never read directly insrc/.Design decisions worth reviewing:
internal-model-v1fails both predicates, so bypassing onlyisGeminiModelwould leave the second guard throwing and the escape hatch still broken.!isGemini2OrAboveguard, so a bare|| modelCheckDisabledwould letgemini-1.5-prothrough with the flag set — which adk-python explicitly forbids (url_context_tool.py:54-55raises before the bypass is considered).google_maps_grounding_tool.ts:29-35already encodes the same "Gemini 1.x is rejected ahead of the bypass" shape.gemini-1.5-flash+ the flag appends the tool there; a plain||reproduces that. The asymmetry with the URL context tool is in the reference implementation, not an oversight, and it tracks capability: code execution works on Gemini 1.5, url_context does not exist there at all. Net effect to be aware of when reviewing: with the flag set,BuiltInCodeExecutoradmitsgemini-1.5-*andUrlContextToolstill rejects it.llmRequest.model &&guard is retained (local convention wins). adk-python has no such guard, somodel=Noneplus the flag would append a code-execution tool there. Keeping it preserves the behaviour pinned by the pre-existing... not supported for model undefinedtest and avoids attaching a tool to a request that names no model. The flag suppresses a validation; it does not turn a previous early-return into a positive capability claim. Not observable across the language boundary for any request that actually sets a model.Url context tool cannot be used in Gemini 1.x.wording was deliberately not adopted — it would break a pre-existing assertion for no benefit._is_managed_agentis not ported. adk-js has no equivalent ofLlmRequest._is_managed_agent(noisManagedAgentanywhere incore/src), so that disjunct of Python's condition is out of scope.Deliberately out of scope (not silently dropped):
core/src/utils/model_name.tsis untouched, so adk-js'sisGemini2OrAbovestill lacks Python's EAP-model handling;google_search_tool.ts:54has the same missing bypass and is, after this PR, the last built-in tool whose model-id gate still ignores the flag;vertex_rag_retrieval_tool.tshas no model gate at all. Each is a separate change. Of the three, only themodel_name.tsEAP gap has an open PR on this fork (#372); thegoogle_search_tool.tsbypass is queued but unimplemented, and is not folded in here to keep this diff to the two call sites the task names.Collision check (required before implementation).
gh pr list --state open --limit 200plus a per-PR file scan forbuilt_in_code_executor|url_context_tool|utils/model_namereturned three adjacent-but-distinct PRs, none of which implements this bypass:model_name.tsand adds EAP cases to both of the same test files. Different feature (EAP name recognition inside the predicate); this PR changes neither the predicate nor its behaviour.as GenerateContentConfigcast onurl_context_tool.ts:59, below the guards touched here. Textually adjacent, semantically unrelated.url_context_tool_test.tsonly.Not stacked on any of them: neither the source hunks nor the new test cases depend on their changes, and branching from
mainkeeps this reviewable on its own. Textual conflicts with #372/#413 in the test files are possible at merge time and are trivial (adjacentitblocks).Note on
as never(revised after complexity review). The two new url-context cases passtoolContext: {} as never, matching the six pre-existing cases in the same file and the analogous cases inenterprise_web_search_tool_test.ts:126/google_maps_grounding_tool_test.ts:99. An earlier revision built a realContext->InvocationContext->LlmAgent/createSession/PluginManagergraph to avoid the cast; that was removed.UrlContextTool.processLlmRequestdestructures{llmRequest}only, so the graph populated a parameter the code under test never reads while coupling a model-id gating test to three unrelated constructors. The root cause is upstream —ToolProcessLlmRequest.toolContext(core/src/tools/base_tool.ts:26) is declared required but is unused by every built-in tool's override — and fixing that signature is a separate change, not something to work around with a stub graph in one file. This PR therefore adds noany,@ts-expect-errororeslint-disable, and no new kind of cast.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.
Four new cases; no existing test case was modified, skipped, or deleted.
core/test/code_executors/built_in_code_executor_test.tsinternal-model-v1when the check is disabled;... not supported for model undefinedwhen the model is unset and the check is disabled (pins the retainedllmRequest.model &&guard).core/test/tools/url_context_tool_test.ts3. addsurlContextforinternal-model-v1when the check is disabled (that id fails both predicates, so one case covers both guards); 4. still rejectsgemini-1.5-prowithrequires Gemini 2 or abovewhen the check is disabled.One test-fixture change beyond the four new cases, called out deliberately. Both suites now pin the flag off with
vi.stubEnv(MODEL_ID_CHECK_ENV_VAR, undefined)inbeforeEach(vi.unstubAllEnvs()inafterEach), matching existing usage incore/test/telemetry/setup_test.tsanddev/test/server/adk_api_server_test.ts. The save-and-restore idiom used by the sibling suites restores the ambient value, which is not sufficient here: because the source now honours the flag, a developer withADK_DISABLE_GEMINI_MODEL_ID_CHECK=trueexported in their shell would see two pre-existing cases fail (throw error if model is invalid,throws for unsupported (non-Gemini) model) — those assertions are only valid with the flag off. Measured before the fixture change:and after it, green in both directions:
No existing assertion was weakened: the two cases above still assert the same throw, they are now simply hermetic with respect to the environment variable this PR makes them sensitive to.
Proving the tests can fail. Every new test was run against mutated source and observed to FAIL:
|| modelCheckDisabled(executor)expected [Function] to not throw an error but 'Error: Gemini code execution tool is …' was thrownllmRequest.model &&(executor)expected [Function] to throw an error&& !bypassModelCheckfrom both url-context guardsURL context tool is not supported for model internal-model-v1&& !bypassModelCheckfrom the second guard onlyURL context tool requires Gemini 2 or above, but got internal-model-v1&& !isGemini1Model(...)frombypassModelCheckpromise resolved "undefined" instead of rejectingThe 3rd and 4th rows are why case 3 alone is sufficient to cover both guards. Note case 2 does carry mutation signal: with the flag off the pre-existing
model is not providedcase passes either way (becauseisGemini2OrAbove(undefined)is false), so case 2 is the only test that pins the model guard once the bypass is on.Coverage of the two changed source files (v8, both files scoped):
100% branch coverage; every line added by this PR is covered. The uncovered lines are pre-existing code this PR does not touch: the
isBuiltInCodeExecutortype guard (29-37) andUrlContextTool.runAsync(32-33).Static checks (on the pushed commit):
npm run ts:checkis red onmainfor 41 unrelated files (281 errors); this PR neither adds to nor fixes that count, verified by running it on a stashed tree.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
No integration fixture is warranted: this is a conditional change in existing code paths with no I/O, no network and no cross-component wiring, and the unit tests drive the real
BuiltInCodeExecutor/UrlContextToolobjects (no mocks, no stubbed model-name helpers) through the publicprocessLlmRequestAPI. To reproduce the original bug and confirm the fix by hand: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.
CI
All checks green on
run-testsfor ubuntu-latest, macos-latest and windows-latest (commit2ac42f8c).macos-latest failed twice before passing, both times with the same unrelated error:
Test timed out in 40000msintests/integration/app_loader/app_loader_test.ts:82(windows-latest was then cancelled by matrix fail-fast, not failed; ubuntu-latest passed every time). Logged rather than papered over, with the diagnosis:BuiltInCodeExecutor,UrlContextTool,URL_CONTEXT,urlContextorcodeExecutionanywhere undertests/integration/app_loader/.npm installper fixture at test time, so its runtime is registry/network-bound rather than code-bound; a cold macOS runner exceeding the 40s hook budget is the expected failure mode. Running it locally fails at the samenpm installstep for an unrelated environment reason, confirming the install is what the test spends its time on.Not fixed here, and deliberately not queued as new work: #405 (align integration install hooks on the project-wide hook timeout) and #407 (pin the integration-fixture install mode) already modify this exact file for this exact problem.