Fix: match only gemini-1.<digit> in isGemini1Model (adk-python parity) - #617
Open
AmaadMartin wants to merge 1 commit into
Open
Fix: match only gemini-1.<digit> in isGemini1Model (adk-python parity)#617AmaadMartin wants to merge 1 commit into
AmaadMartin wants to merge 1 commit into
Conversation
isGemini1Model used a bare `startsWith('gemini-1')` prefix test with no
digit boundary, so it accepted any id whose version field merely begins
with the character `1`. A future `gemini-10.0-pro` would be routed down
the legacy Gemini 1.x paths in GoogleSearchTool, VertexAiSearchTool,
applyGoogleMapsGrounding and applyEnterpriseWebSearch, and would also be
classified as both Gemini 1.x and Gemini 2.0+ at the same time.
Replace the prefix test with the `^gemini-1\.\d+` regex adk-python's
is_gemini_1_model already uses, hoisted to a module-level non-global
constant so `.test()` stays stateless. This makes the existing doc
comment ("using regex patterns") honest and restores mutual exclusivity
with isGemini2OrAbove.
Behaviour is unchanged for every real Gemini 1.x id; only gemini-10.0-pro,
gemini-1, gemini-1-pro, gemini-1. and gemini-1x-foo flip to false.
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:
isGemini1Model— the switch that routes a request down ADK's legacy Gemini 1.x code paths — used a bare prefix test with no digit boundary:That accepts any model id whose version field merely begins with the character
1, and accepts a bare major version with no minor at all. adk-python'sis_gemini_1_model(src/google/adk/utils/model_name_utils.py) requires a literal.followed by at least one digit:re.match(r'^gemini-1\.\d+', model_name). So the two SDKs disagree on what "Gemini 1.x" means.Five input classes are misclassified today:
gemini-10.0-progemini-1gemini-1-progemini-1.gemini-1x-foogemini-10.0-prois the exact shape of a real future release id. The moment such a model ships, adk-js silently routes it down the Gemini 1.x paths with no code change and no warning:GoogleSearchToolwould append the legacygoogleSearchRetrieval: {}and throwGoogle search tool can not be used with other tools in Gemini 1.x., andapplyGoogleMapsGrounding,VertexAiSearchTool.processLlmRequestandapplyEnterpriseWebSearchwould each throw their Gemini 1.x-only errors for a Gemini 10 model.It also breaks a structural invariant adk-python pins in
TestModelNameUtilsIntegration::test_model_classification_consistency: a model must never be classified as both Gemini 1.x and Gemini 2.0+. Todaygemini-10.0-prosatisfies both, becauseparseVersion('10')yields major 10 ≥ 2.The existing doc comment already claimed the behaviour the code did not implement — "Check if the model is a Gemini 1.x model using regex patterns".
Solution: Replace the prefix test with the same regex adk-python uses, hoisted to a module-level constant next to the existing
MODEL_NAME_PATTERN:Two files change, +76/−1. Notes on the choices:
/gflag. A global-flagged regex carries mutablelastIndexstate across.test()calls, which would make this pure predicate return alternating results for the same input. A non-global literal is stateless and safe to hoist to module scope.MODEL_NAME_PATTERNis deliberately left as astring. It is consumed byString.prototype.matchand belongs toextractModelName's hunk; harmonising the two would be an out-of-scope edit.modelString: string. adk-python acceptsOptional[str]and short-circuits on falsy input, but all four call sites already guard withif (!llmRequest.model) return;, and''returnsfalseunder the new regex without an explicit guard. Anundefinedbranch would be dead code.\dmatches Unicode decimal digits, JavaScript's is ASCII-only. ASCII-only is the desired behaviour for model identifiers, so this is not chased with theuflag or\p{Nd}.gemini-1.*model usegemini-1.5-*orgemini-1.0-proand are unaffected.Scope. Only
isGemini1Modelchanges.extractModelName,isGeminiModel,isGemini2OrAbove,isGemini3xFlashLive,parseVersionandisGeminiModelIdCheckDisabledare untouched, the four call sites are untouched, andisGemini1Modelstays unexported fromcore/src/common.tsandcore/src/index.ts. BecauseextractModelNamedoes not yet strip provider prefixes, adk-python's provider-prefixed cases (gemini/gemini-1.5-flash,vertex_ai/gemini-1.5-flash,openrouter/google/gemini-1.5-pro:online) are intentionally excluded from the test plan — porting them here would force an out-of-scope edit.Prior-art / collision check. Before writing anything I scanned all 516 open PRs on the fork (
gh pr list --limit 1000) and inspected the three that touchcore/src/utils/model_name.ts. PR #615 (fix/model-name-eap-and-path-parity) bundles an equivalent Gemini-1 boundary fix, but it is stacked three deep on separate tasks (#615 → #471extractModelNamewidening → #372 EAP gate →main) and carries those unrelated changes with it. This PR is the standalone,main-based version of just the boundary fix, reviewable and revertable on its own. If #615 lands first this becomes a no-op and can be closed; if this lands first, #615'smodel_name.tshunk becomes redundant.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.
A new
describe('isGemini1Model', ...)block was added tocore/test/utils/model_name_test.ts. The two pre-existing describe blocks (isGemini2OrAbove,isGemini3xFlashLive) are unchanged — no existing test was edited, weakened, skipped or deleted. The block follows the file's existing table-driven style and covers 16 inputs plus 2 invariants:gemini-1.5-flash,gemini-1.0-pro,gemini-1.5-pro-preview,gemini-1.9-experimental, and two Vertex path forms (exercising composition with the unmodifiedextractModelName).gemini-2.5-flash,claude-3-sonnet,my-gemini-1.5-model(present but not anchored),'', and a path-basedgemini-2.5-flash.test_model_classification_consistencyinvariant), and every Gemini 1.x model is also a Gemini model.Targeted runs (no full-repo suite):
The four tool suites are the integration-level regression signal for the changed predicate; all pass unchanged. New-line and new-branch coverage of the changed statement is 100% — the test block executes it on both outcomes.
Falsifiability proof (test run against the unfixed code). The new block was written before the fix and run against the original
startsWith('gemini-1')implementation. It failed with exactly the predicted signature — the five boundary cases, plus the mutual-exclusivity invariant:The other 10 cases pass both before and after, which is expected — they pin the behaviour the change must not alter.
Manual End-to-End (E2E) Tests:
No E2E test is applicable:
isGemini1Modelis a pure, module-internal, string-in/boolean-out predicate with no I/O, network or DI surface, and exercising the headline case end-to-end would require a publishedgemini-10.xmodel, which does not exist. The behaviour table above, reproduced as executable assertions, is the complete verification.To reproduce the fix locally:
Standalone sanity check of the regex itself:
Lint and format gates on the changed files pass:
Disclosure on
npm run ts:check: it is red on this branch, but it is equally red on the base commit —npx tsc --noEmit --pretty falsereports 281 errors both with and without this change, none of them in either file touched here (all are pre-existingBASE_AGENT_SIGNATURE_SYMBOLassignability errors incore/test/a2a/*and similar). This PR neither introduces nor fixes any of them; fixing them is out of scope.No suppressions of any kind were added:
git diff <base> -U0 | grep -E '@ts-expect-error|@ts-ignore|eslint-disable|as any|: any|v8 ignore'returns nothing.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
All checks green on the pushed commit:
run-testspasses on ubuntu-latest, windows-latest and macos-latest, plus the standalonerun-tests,check-licenseandauto-assignjobs.macos-latest failed on the first two attempts, both times on a pre-existing flake unrelated to this change, and passed on re-run:
Recording it here rather than quietly re-running, since it is worth knowing the job is unstable:
tests/integration/app_loader/is not in this diff, which touches onlycore/src/utils/model_name.tsandcore/test/utils/model_name_test.ts. A regex change to a pure string predicate cannot make an agent-discovery fixture exceed a 40s timeout.it()rather thanbeforeAll— Fix: bill app_loader discovery fixture compilation to beforeAll instead of the first test #560, Fix: stop the app_loader discovery test billing its setup to a test, and make a premature test-server exit self-diagnosing #545, Fix: make the app_loader integration suite independent of the previous run #521, Fix: stop the app_loader discovery test billing its fixture setup to the first it() #506, Fix: stop Vitest SSR-transforming the compiled agent bundle in app_loader integration tests #499, Fix: attribute cold AgentLoader discovery cost to beforeAll to stop macOS CI flake #260, Fix: raise app_loader integration-test timeout to 60s to stop CI flake #256, Fix: bound AgentLoader bundle imports so app_loader discovery test stops timing out on Windows/macOS CI #247, Fix: stabilize app_loader integration test timeouts and stop matrix fail-fast #235.No change was made to accommodate it; fixing it here would mean editing an unrelated integration test in this diff.