Skip to content

Test: pin the EAP pattern boundaries and version-path parity negatives (stacked on #372) - #618

Open
AmaadMartin wants to merge 4 commits into
fix/gemini-eap-model-name-parityfrom
fix/gemini-eap-model-detection
Open

Test: pin the EAP pattern boundaries and version-path parity negatives (stacked on #372)#618
AmaadMartin wants to merge 4 commits into
fix/gemini-eap-model-name-parityfrom
fix/gemini-eap-model-detection

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:

Stacked on #372 — read that first. Base branch is fix/gemini-eap-model-name-parity, not main.

Collision check (run before any code was written).
gh pr list --repo AmaadMartin/adk-js --state open --limit 1000 surfaced two live PRs on this
ground:

Problem: #372 fixes the defect, but its suite pins the happy shape of the EAP pattern more
than its boundaries. Several ways of loosening the regex, and several ways of breaking the
version-parsing path that the EAP branch now sits in front of, still pass on #372's head. Concretely,
none of these were covered: a suffix that merely starts with early-exp
(gemini-flash-early-experiment), a non-digit character after early-exp (gemini-flash-early-expX),
a missing <variant> segment combined with a numeric suffix (gemini-early-exp3), the adk-python
parity negatives for the version path (gemini-one, gemini-0.9-test, gemini-2., gemini-), and
the if (!modelString) guard, which no test reached.

Solution: 17 added lines in core/test/utils/model_name_test.ts, appended only — no existing
entry, ordering or assertion was edited or removed. Two follow-up commits then address a complexity
review of the whole stack (see Complexity review below); together they take the stack from
4 files / +124-3 to 4 files / +99-2.

Added case Placed in Property it pins
gemini-flash-early-experiment, gemini-flash-early-expX nonEapModels pattern stays anchored at the end; early-exp is exact, not a prefix
gemini-early-exp3 nonEapModels a <variant> segment is mandatory even with a numeric suffix
gemini-one, gemini-0.9-test, gemini-2., gemini- invalidModels adk-python parity negatives on the version path (unparseable version, major < 2)
isGemini2OrAbove('') new it in invalid models the falsy guard's contract
gemini-2.5-flash-early-exptrue new it in EAP models the EAP branch only short-circuits on true, never returns its own verdict

That last one is the load-bearing one and is worth spelling out. The natural way to write this
feature wrong is if (name.includes('early-exp')) return EAP_PATTERN.test(name); — which reads fine,
passes every other test in the file, and silently demotes a versioned name carrying the suffix
from true to false. It is the only test that catches that, and it is what enforces the change's
monotonicity property (nothing that returned true before may return false now).

I did not reshape #372's implementation. The elaborated design I was given sketches a private
isGeminiEapModel(modelString) wrapper that calls extractModelName itself; #372 instead tests the
pattern against the already-extracted name inside isGemini2OrAbove. Same regex, same ordering
relative to the prefix check and version parse, identical truth table — rewriting a sibling PR's
just-reviewed hunk into an equivalent shape is churn, not an improvement.

One deliberate, pre-existing divergence from adk-python, inherited from #372 and left alone: Python's
$ also matches immediately before a trailing newline, so re.match accepts
'gemini-flash-early-exp\n'; the JS $ without the m flag does not. JS is stricter and more
correct here, so it is not emulated and no test asserts the Python quirk.

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/utils/model_name_test.ts \
  core/test/tools/url_context_tool_test.ts \
  core/test/code_executors/built_in_code_executor_test.ts
  -> 63 passed (63)      # model_name 50, url_context 8, built_in_code_executor 5
npx eslint core/test/utils/model_name_test.ts core/src/utils/model_name.ts   -> exit 0
npx prettier --check core/test/utils/model_name_test.ts                      -> clean
npm run build                                                                -> exit 0

Proof each new test can fail. Every case was run against a targeted mutation of the exact line it
pins; each failed, and the source was restored and re-verified green (50/50) after each.

Mutation to core/src/utils/model_name.ts Tests that FAILED
A — drop the trailing $ from the pattern gemini-flash-early-experiment, gemini-flash-early-expX (+ #372's …-exp-001)
B — [a-z0-9_]+(?:-[a-z0-9_]+)*-early-exp(?:[a-z0-9_]+-)*early-exp (variant optional) gemini-early-exp3 (+ #372's gemini-early-exp)
C — return parsedVersion.valid && parsedVersion.major >= 2return parsedVersion.valid gemini-0.9-test (+ gemini-1.5-pro, gemini-1.0-pro)
D — same line → return !parsedVersion.valid || parsedVersion.major >= 2 gemini-one, gemini-2., gemini- (+ 9 others)
E — if (!modelString) return falsereturn true should return false for an empty model string (only that one)
G — EAP branch → if (name.includes('early-exp')) return EAP_PATTERN.test(name) should admit a versioned name carrying the EAP suffix (only that one)

Example failure output (mutation A):

× isGemini2OrAbove > EAP models > should return false for non-EAP model: gemini-flash-early-experiment
× isGemini2OrAbove > EAP models > should return false for non-EAP model: gemini-flash-early-expX
Tests  3 failed | 47 passed (50)

Two honest caveats rather than overclaiming:

  • The empty-string test pins a contract, not a latent bug. Inverting the guard (mutation E) fails
    it, but deleting the guard outright leaves all 50 green — '' falls through to
    ''.startsWith('gemini-') and returns false anyway. So it buys branch coverage of that guard and
    locks its behaviour; it is not a regression signal for a reachable defect. Stated plainly so nobody
    counts it as more than it is.
  • gemini-2.5-flash-early-exp is a genuine signal, but only against mutation G. Widening the
    character class to include . (so a versioned name does match the EAP pattern) leaves it passing,
    because the answer is true down either branch; that variant is caught by Fix: recognise Gemini EAP model names in isGemini2OrAbove (adk-python parity) #372's existing
    gemini-1.5-flash-early-exp negative instead. The two tests are complements.

Repo-wide npm run ts:check is red on this branch — and equally red on the base, unrelated to this
change.
npx tsc --noEmit --pretty false reports 280 errors with this diff and the same 280 on
#372's pristine head
, 0 of them in any file this PR touches. They are all the
core/dist/types/... is not assignable to core/src/... dual-identity artifact that appears once a
built core/dist exists in the working tree (removing core/dist makes it worse, 741, since
@google/adk then resolves to nothing). npx tsc --noEmit -p core/tsconfig.json — the project that
actually covers core/src — exits 0. Nothing here is mine to fix and I have not touched it.

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

Run against the real compiled build (npm run build, then import from core/dist/esm/index.js — no
mocks, no stubs), exercising the predicate and both downstream gates that the EAP fix unlocks:

import {
  isGemini2OrAbove,
  UrlContextTool,
  BuiltInCodeExecutor,
} from './core/dist/esm/index.js';

isGemini2OrAbove('gemini-flash-early-exp'); // true
isGemini2OrAbove('gemini-flash-early-expX'); // false

const req = {
  model: 'gemini-flash-early-exp',
  contents: [],
  toolsDict: {},
  liveConnectConfig: {},
};
await new UrlContextTool().processLlmRequest({
  llmRequest: req,
  toolContext: undefined,
});
// req.config.tools -> [{"urlContext":{}}]     (previously threw)

const req2 = {
  model: 'gemini-flash-early-exp',
  contents: [],
  toolsDict: {},
  liveConnectConfig: {},
};
new BuiltInCodeExecutor().processLlmRequest(req2);
// req2.config.tools -> [{"codeExecution":{}}] (previously threw)

Observed, in full:

true  "gemini-flash-early-exp"                     false "gemini-1.5-pro"
true  "gemini-flash-early-exp3"                    false "gemini-one"
true  "gemini-flash-lite-early-exp"                false "gemini-2."
true  "gemini-pro-early-exp"                       false "gemini-"
true  "projects/p/.../models/gemini-pro-early-exp" false "gemini-flash-early-experiment"
true  "gemini-2.5-flash-early-exp"                 false "gemini-flash-early-expX"
true  "gemini-2.5-flash"                           false "gemini-early-exp3"
                                                   false ""
UrlContextTool  -> [{"urlContext":{}}]
BuiltInCodeExec -> [{"codeExecution":{}}]

Per the design note, this probe is not committed as a test file: the two consumers are gated by the
same predicate and already have their own suites (#372 extends both), and a queued follow-up will be
editing exactly those two files.

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.


Complexity review — findings addressed (two rounds)

A complexity audit of the full stack (fcfd0436..HEAD) raised two items. Both sit in #372's commits
rather than in my test additions, but since they are in this branch's delta I fixed them here.

1. makeToolContext() in url_context_tool_test.ts — factory removed, test case kept
(net +11 lines instead of the original +37).
The finding was that a 15-line factory with one
caller built an InvocationContext the code under test never reads (processLlmRequest
destructures only {llmRequest}), expanding a one-line import into a ten-line block.

My first attempt at this deleted the factory and the test case, on the reasoning that an
assertion needing 15 lines of inert scaffolding was not worth its cost. That was wrong and the
follow-up review was right to block it.
url_context_tool.ts:43 is the one call site of
isGemini2OrAbove that throws on a false negative, and
URL context tool requires Gemini 2 or above, but got gemini-flash-early-exp is precisely the
user-visible symptom this change exists to remove. Deleting the case left that wiring with no
regression guard — the model_name_test.ts cases cover the predicate, not the tool's use of it.
Coverage at a sibling consumer is not a substitute for the one that throws.

The case is restored in the one-line form the file's six other call sites already use: no helper,
no new imports, +11 lines rather than +37.

Suppression disclosure (one site, required by the guidelines). That restored case passes
toolContext: {} as never, so this branch adds one unchecked cast. Flagging it explicitly
rather than letting it pass unremarked. The concrete reason: ToolProcessLlmRequest.toolContext
is a required Context, UrlContextTool.processLlmRequest provably never reads it, and the only
cast-free alternative is reconstructing the very InvocationContext factory this finding asked
to delete — so avoiding the cast and satisfying the finding are mutually exclusive. It is the
established idiom for this parameter (6 pre-existing occurrences in this file, 28 across
core/test), so it introduces no new pattern and waives no check that is not already waived on
the adjacent lines. No other suppression appears anywhere in the stack.

Proof the restored case is load-bearing. Deleting the EAP short-circuit from isGemini2OrAbove
makes it fail with the production error verbatim:

× UrlContextTool > processLlmRequest > adds urlContext for an EAP model
  → URL context tool requires Gemini 2 or above, but got gemini-flash-early-exp
Tests  1 failed | 7 passed (8)

2. Duplicated EAP prose in model_name.ts — collapsed (-4 lines). The convention and both
examples were spelled out on EAP_MODEL_NAME_PATTERN and again in the isGemini2OrAbove doc. The
normative description now lives once, next to the pattern; the function references it in plain prose
(not {@link} — TypeDoc runs with --treatWarningsAsErrors and a link from an exported symbol to a
non-exported one warns). Also dropped the "without the g flag so .test() stays stateless" note,
which documented the absence of a bug. No logic changed — the source diff is comments only plus
#372's original three-line short-circuit.

Everything the reviewer explicitly cleared was left alone: the regex is still verbatim the upstream
pattern, the check still precedes the startsWith('gemini-') guard, and the inlined form is kept
rather than ported into a wrapper.


CI status: absent (not green) — validated locally instead

No test job ran on this PR, and none will. Every test-bearing workflow in .github/workflows is
gated on pull_request: branches: [main] (cross-language-integration.yml, license-check.yml);
this PR's base is the stacked branch fix/gemini-eap-model-name-parity, so they never trigger. The
only check that reported is auto-assign, which comes from auto-assignment.yml
(pull_request: types: [opened], no branch filter) and is not validation of anything.

Rather than call that green, here is the local run on the exact pushed commit 11e47a5, working
tree clean:

npx vitest run --project unit:core \
  core/test/utils/model_name_test.ts \
  core/test/tools/url_context_tool_test.ts \
  core/test/code_executors/built_in_code_executor_test.ts   -> 63 passed (63)
npm run build                                               -> exit 0
npm run lint                                                -> exit 0
npx eslint <the four touched files>                         -> exit 0
npx prettier --check <the touched files>                    -> clean

Full CI will run against these tests once #372 merges to main and this branch is retargeted.

Amaad Martin added 4 commits August 3, 2026 22:30
…tives

Extends the isGemini2OrAbove suite with the edge cases the EAP change
leaves unpinned, so a later loosening of the pattern cannot pass silently:

- regex shape: 'gemini-flash-early-experiment' and 'gemini-flash-early-expX'
  fail if the pattern loses its trailing anchor; 'gemini-early-exp3' fails if
  the <variant> segment becomes optional.
- version path (adk-python parity negatives): 'gemini-one', 'gemini-0.9-test',
  'gemini-2.' and 'gemini-' fail if an unparseable version is admitted or the
  major >= 2 bound is dropped.
- the falsy guard, which no test reached before.
- 'gemini-2.5-flash-early-exp' -> true, pinning that the EAP branch only
  short-circuits on a match rather than returning its own verdict, which
  would demote versioned names carrying the suffix.

Every case was verified to fail under a targeted mutation of the line it
pins.
…L-context suite

The EAP case added a 15-line makeToolContext() factory with a single caller,
pulling five more imports into the file, to build an InvocationContext that
UrlContextTool.processLlmRequest never reads - it destructures only
{llmRequest}. The assertion it enabled was already covered elsewhere, so the
scaffolding was cost without signal.

The alternative of passing `toolContext: {} as never`, as the file's six other
call sites do, was rejected: it trades a real object for an unchecked cast, and
the cast is only avoidable here by not needing the argument at all.

EAP admission at a consumer gate stays pinned by
built_in_code_executor_test.ts, which needs no context and no cast, and the
predicate itself is exhaustively covered in utils/model_name_test.ts.
The convention and its two examples were spelled out both on
EAP_MODEL_NAME_PATTERN and again in the isGemini2OrAbove doc. Keep the
normative description next to the pattern and have the function reference it,
mirroring how adk-python cross-references its helper.

Also drops the note that the literal carries no `g` flag: it documented the
absence of a bug that a reader of a non-`g` literal would not suspect.
Removing the inert makeToolContext() scaffolding in the previous commit took
the test case with it, which went too far. UrlContextTool is the call site that
*throws* on a false negative -- "URL context tool requires Gemini 2 or above,
but got gemini-flash-early-exp" is the user-visible symptom this change exists
to remove -- and no test guarded that wiring once the case was gone. The unit
tests on isGemini2OrAbove cover the predicate, not the tool's use of it.

The case is back in the one-line form the file's six other call sites already
use, so it costs 11 lines with no helper and no new imports, rather than the
37 the factory version cost.

Verified to fail without the fix: deleting the EAP short-circuit from
isGemini2OrAbove makes this test fail with the exact production error above.
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