Skip to content

Fix: honour ADK_DISABLE_GEMINI_MODEL_ID_CHECK in GoogleSearchTool - #614

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/google-search-tool-model-id-check-bypass
Open

Fix: honour ADK_DISABLE_GEMINI_MODEL_ID_CHECK in GoogleSearchTool#614
AmaadMartin wants to merge 2 commits into
mainfrom
fix/google-search-tool-model-id-check-bypass

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 4, 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:
    Problem: ADK ships an escape hatch, ADK_DISABLE_GEMINI_MODEL_ID_CHECK, for users pointing ADK at a Gemini-backed endpoint whose model id does not start with gemini- (a proxied, aliased or internally-named deployment such as internal-model-v1). GoogleSearchTool.processLlmRequest never consulted it: it gated purely on isGeminiModel(llmRequest.model) and otherwise threw Google search tool is not supported for model <model>, so the built-in Google Search tool could not be attached to such a model at all.

This is both a cross-language parity gap and an internal inconsistency:

  • Parity source: adk-python src/google/adk/tools/google_search_tool.py, process_llm_request, reads the flag into model_check_disabled and ORs it into the model-family gate.
  • Three sibling adk-js tools already honour the same flag — core/src/tools/vertex_ai_search_tool.ts, core/src/tools/enterprise_web_search_tool.ts, core/src/tools/google_maps_grounding_tool.ts.

Solution: Read the flag through the existing isGeminiModelIdCheckDisabled() helper and OR it into the Gemini-family gate, exactly as the three sibling tools do:

const modelCheckDisabled = isGeminiModelIdCheckDisabled();
...
if (isGeminiModel(llmRequest.model) || modelCheckDisabled) {
  llmRequest.config.tools.push({googleSearch: {}});
  return;
}

Deliberately in scope / out of scope:

  • Gemini 1.x precedence is preserved. The flag is not ORed into the isGemini1Model branch, matching adk-python where is_gemini_1_model is the if and the disjunct lives in the elif. A Gemini 1.x request still gets googleSearchRetrieval and still raises "can not be used with other tools in Gemini 1.x." whether or not the flag is set.
  • adk-python's third disjunct _is_managed_agent(llm_request) is deliberately NOT ported. adk-js has no isManagedAgent equivalent anywhere in core/src, and inventing one is out of scope for a bug fix. Parity here is limited to the model_check_disabled term.
  • No new abstraction. No applyGoogleSearch() free function was extracted "to match" the maps/enterprise tools; that is a refactor, not this fix.
  • Sibling files with the same gap are out of scope. url_context_tool.ts and built_in_code_executor.ts are handled separately; this PR changes exactly two files.
  • Shared predicates (core/src/utils/model_name.ts, core/src/utils/env_aware_utils.ts) are unchanged, no process.env is read directly in src/, no export changes, no new error types. The change is strictly permissive and opt-in, so it is not a breaking change: with the variable unset, behaviour and error text are byte-identical to before.

Collision check (required before implementation): gh pr list --repo <fork> --state open --limit 1000 returned 513 open PRs; filtering by files[].path for google_search_tool returned #178, #293, #332, #413, #487, #514. All six are repo-wide typecheck/lint cleanups, none implements this behaviour: only #332 touches core/src/tools/google_search_tool.ts, and it merely drops a now-redundant as GenerateContentConfig cast on a different line. #470 (honour ADK_DISABLE_GEMINI_MODEL_ID_CHECK in BuiltInCodeExecutor and UrlContextTool) is the closest sibling and touches only the four files for those two components — no overlap. This branch is therefore based directly on main, not stacked.

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.

Three new cases were added to core/test/tools/google_search_tool_test.ts inside a nested describe('ADK_DISABLE_GEMINI_MODEL_ID_CHECK'). No existing test was edited, weakened, skipped or deleted — in particular 'throws for unsupported (non-Gemini) model' is untouched and remains the regression guard for the un-bypassed path.

  1. adds googleSearch for a non-Gemini model when the check is disabled — flag 'true', model internal-model-v1, asserts [{googleSearch: {}}]. Mirrors adk-python's test_process_llm_request_with_non_gemini_model_and_disabled_check.
  2. keeps Gemini 1.x handling when the check is disabled — flag 'true', model gemini-1.5-pro, asserts [{googleSearchRetrieval: {}}]. Pins the precedence invariant, which no existing test covered.
  3. still throws for a non-Gemini model when the value is falsy — flag 'false', asserts the original error still throws. This exercises the error path under the flag, proving the bypass is truthiness-gated rather than presence-gated.

Two deliberate deviations from the surrounding file style, both in the safer direction and both confined to the new tests:

  • The new tests build a real Context (real InvocationContext, LlmAgent, Session, PluginManager) via a local makeToolContext() helper rather than copying this file's toolContext: {} as never convention. as never is an unchecked cast, so the new code introduces none; the six pre-existing call sites were left alone (a repo-wide cleanup of those casts is already in flight in a separate PR).
  • Env handling uses vi.stubEnv + afterEach(vi.unstubAllEnvs) — already used in core/test/telemetry/setup_test.ts and core/test/code_executors/agent_engine_sandbox_code_executor_test.ts — instead of duplicating a ten-line manual save/restore block in each test. It gives the same leak-proof restore guarantee (including on assertion failure) without the duplication.

Command: npx vitest run --project unit:core core/test/tools/google_search_tool_test.ts10 passed (7 pre-existing + 3 new).

Coverage. Measured with --coverage.include='core/src/tools/google_search_tool.ts': 100% branch, 95.23% line/statement. The only uncovered lines are 31–32, the body of the pre-existing runAsync() built-in stub, which this PR does not touch and which had no test before this change. Both lines added by this PR — the const modelCheckDisabled = ... and the modified gate — are at 100% line and 100% branch coverage, including the newly reachable true value of the new disjunct.

Proof the new tests can fail (mutation testing). Each mutation was applied to core/src/tools/google_search_tool.ts, the targeted suite re-run, then the source restored. Every mutation is killed, and each is killed by a different new test:

# Mutation Result
1 Revert the fix: if (isGeminiModel(llmRequest.model)) { Test 1 FAILS — Error: Google search tool is not supported for model internal-model-v1. Tests 2 and 3 still pass (that is the point of test 2: it pins behaviour the fix must not change).
2 Wrongly OR the flag into the Gemini 1.x branch: if (isGemini1Model(llmRequest.model) || modelCheckDisabled) { Test 1 FAILS — AssertionError: expected [ { googleSearchRetrieval: {} } ] to deeply equal [ { googleSearch: {} } ].
3 Let the flag suppress the Gemini 1.x branch: if (isGemini1Model(llmRequest.model) && !modelCheckDisabled) { Test 2 FAILS — AssertionError: expected [ { googleSearch: {} } ] to deeply equal [ { googleSearchRetrieval: {} } ].
4 Gate on presence instead of truthiness: const modelCheckDisabled = process.env.ADK_DISABLE_GEMINI_MODEL_ID_CHECK !== undefined; Test 3 FAILS — promise resolved "undefined" instead of rejecting.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

No integration-test fixture was added: this is an in-process gate change on a built-in tool that only mutates a request object — there is no transport, service or cross-component wiring to exercise, so a fixture would cost CI time for no signal.

Instead, the reproduction from the bug report was run against the built package (npm run build, importing @google/adk from dist, no mocks, no test framework), which confirms every row of the expected behaviour table:

import {GoogleSearchTool} from '@google/adk';
process.env.ADK_DISABLE_GEMINI_MODEL_ID_CHECK = 'true';
const req = {
  model: 'internal-model-v1',
  config: {tools: []},
  contents: [],
  toolsDict: {},
  liveConnectConfig: {},
};
await new GoogleSearchTool().processLlmRequest({
  llmRequest: req,
  toolContext: {},
});
console.log(req.config.tools);

Observed (this branch):

flag=true      model=internal-model-v1 -> tools=[{"googleSearch":{}}]
flag=1         model=internal-model-v1 -> tools=[{"googleSearch":{}}]
flag=<unset>   model=internal-model-v1 -> THREW: Google search tool is not supported for model internal-model-v1
flag=false     model=internal-model-v1 -> THREW: Google search tool is not supported for model internal-model-v1
flag=true      model=gemini-1.5-pro    -> tools=[{"googleSearchRetrieval":{}}]
flag=<unset>   model=gemini-2.0-flash  -> tools=[{"googleSearch":{}}]

On main the first two rows throw instead. To reproduce: export ADK_DISABLE_GEMINI_MODEL_ID_CHECK=true, then run the snippet above.

Other checks run on the pushed commit: npm run build (clean), npx eslint on both touched files (clean), npx prettier --check on both touched files (clean). npm run ts:check is red repo-wide on main today; the count for core/test/tools/google_search_tool_test.ts is unchanged by this PR at exactly 1 — the pre-existing Type '{ functionDeclarations: never[]; }' is not assignable to type 'never' on the untouched Gemini 1.x multi-tool test, caused by the tools = [] default in the file's makeRequest helper. This PR adds zero new type errors and no suppressions of any kind.

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 status on this PR

All checks pass on the reviewed commit: run-tests, run-tests (ubuntu-latest), run-tests (macos-latest), run-tests (windows-latest), check-license and the Cross-Language Tests job.

run-tests (windows-latest) was intermittently red on earlier runs of this same, unchanged commit, always on a pre-existing Windows flake unrelated to this diff: twice on core/test/code_executors/unsafe_local_code_executor_test.ts > UnsafeLocalCodeExecutor > should execute shell code and return stdout (Test timed out in 5000ms) and once on tests/integration/app_loader/app_loader_test.ts > should discover apps vs agents across directories and standalone files (~41s timeout) — two different unrelated files, both real-subprocess timeouts, 223/244 test files passing each time. Both are already tracked by open PRs (#498 gives the real-subprocess cases in unsafe_local_code_executor_test.ts an explicit 60s timeout; #254 and #246 harden the same Windows shell cases), and both files pass locally on this commit. The job going green on a rerun with no code change confirms the flake rather than a regression; the fix belongs to those PRs, not here.

Amaad Martin added 2 commits August 3, 2026 21:17
GoogleSearchTool.processLlmRequest gated purely on isGeminiModel(), so the
ADK_DISABLE_GEMINI_MODEL_ID_CHECK escape hatch had no effect on it even though
VertexAiSearchTool, EnterpriseWebSearchTool and GoogleMapsGroundingTool all
honour it. Users pointing ADK at a Gemini-backed endpoint with a non-standard
model id could not attach the built-in Google Search tool.

OR the flag into the Gemini-family gate, matching adk-python
src/google/adk/tools/google_search_tool.py. Gemini 1.x precedence and the
un-bypassed error text are unchanged.
The two new bypass cases originally copied this file's `{} as never`
toolContext convention and hand-rolled env save/restore. Build a real
Context (real InvocationContext, LlmAgent, Session, PluginManager) instead,
so the new tests introduce no unchecked cast and run against genuine ADK
plumbing, and use vi.stubEnv/vi.unstubAllEnvs (already used elsewhere in
core/test) instead of duplicating the restore block.

Also pins that a falsy flag value does not bypass the model check. The six
pre-existing tests are untouched.
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