Fix: drop the unnecessary structural cast when clearing labels in Gemini.preprocessRequest - #411
Open
AmaadMartin wants to merge 2 commits into
Open
Fix: drop the unnecessary structural cast when clearing labels in Gemini.preprocessRequest#411AmaadMartin wants to merge 2 commits into
AmaadMartin wants to merge 2 commits into
Conversation
added 2 commits
July 31, 2026 14:07
preprocessRequest clears GenerateContentConfig.labels for the Gemini API backend and leaves it alone for Vertex AI, but neither branch had any test coverage. Add both cases so the asymmetry is pinned before the surrounding code is touched.
…ini.preprocessRequest
@google/genai already declares GenerateContentConfig.labels?: Record<string,
string>, so `(llmRequest.config as {labels?: unknown}).labels = undefined`
asserted a weaker type than the one the compiler had. Write the declared
property directly, matching the uncast sibling write in llm_agent.ts.
No runtime change: labels is still assigned undefined rather than deleted,
preserving parity with adk-python's `llm_request.config.labels = None`.
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:
Gemini.preprocessRequest(core/src/models/google_llm.ts) clears the outgoing request's billinglabelsbefore calling the Google AI Studio backend, and did so through an inline structural assertion:That assertion is not load-bearing, and it is actively harmful:
LlmRequest.configisGenerateContentConfigfrom@google/genai(core/src/models/llm_request.ts:10,36).core/package.jsonpins"@google/genai": "^2.9.0"andpackage-lock.jsonresolves it to exactly2.9.0; that published version declareslabels?: Record<string, string>;atdist/genai.d.ts:4780, inside theexport declare interface GenerateContentConfigblock that opens at line 4661 (verified against the installednode_modulestree, not from memory). Nothing needed asserting.core/src/agents/llm_agent.ts:1072-1078doesllmRequest.config ??= {}; llmRequest.config.labels ??= {};and then indexes into it — compiling today under the same tsconfig. Two sites writing one property at two different levels of type rigour was the inconsistency.as {labels?: unknown}widens the value type fromRecord<string, string>tounknown, so any value at all would have satisfied the assignment. A structural cast that widens a field isas anywearing a hat.Solution: Write the declared property directly:
The diff to
core/srcis exactly one line. The explanatory comment about Google AI Studio API keys is kept verbatim.Why
= undefinedand notdelete:exactOptionalPropertyTypesis not enabled, so assigningundefinedto an optional property is legal understrict: true. Verified by following the wholeextendschain: roottsconfig.json→gts@5.3.1/tsconfig-google.json(which sets onlyallowUnreachableCode,allowUnusedLabels,declaration,forceConsistentCasingInFileNames,lib,module,noEmitOnError,noFallthroughCasesInSwitch,noImplicitReturns,pretty,sourceMap,strict,target);core/tsconfig.jsonadds onlyrootDir/outDir/include/exclude. The only occurrence ofexactOptionalPropertyTypesin this repo is inside theTS_CONFIGstring templateadk createwrites into a scaffolded user project (dev/src/cli/cli_create.ts:33), which does not configure this repository's own compilation.deletewould also be a semantic change (the key would disappear, so'labels' in configflips andObject.keys(config)shrinks) and would diverge from the adk-python reference,src/google/adk/models/google_llm.py, which assignsllm_request.config.labels = None. Parity wins here because the assignment form is observable.Runtime behaviour is unchanged:
labelsis still set toundefined(key present, never deleted) on the Gemini API path, and still left untouched on the Vertex AI path.No suppression of any kind was added — no
@ts-expect-error,@ts-ignore,eslint-disable,any,as any,as never,as unknown as, index signature, or coverage-tool ignore, in either the source or the test. This PR removes an escape hatch; adding one back would defeat the point.git diff <base> -U0 | grep -E '@ts-expect-error|@ts-ignore|eslint-disable|v8 ignore|istanbul ignore|: any\b|as any|as never|as unknown as'returns nothing.Collision check. Ran
gh pr list --state open --limit 500(314 open PRs) and grepped every plausibly adjacent diff forlabels: none of them touch this property, and no open PR modifiespreprocessRequest. Three open PRs (#366, #313, #332) do touchcore/test/models/google_llm_test.ts, but in unrelated regions of the file, so this branches frommainrather than stacking.Explicitly left alone, as separate concerns:
removeDisplayNameIfPresent's own(dataObj as FileData).displayNameassertions in the same file, and theinlineData.displayNamecast inrunner.ts.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.
The primary regression signal for the source change is the type checker itself: if
labelswere not a declared property ofGenerateContentConfig, removing the cast would fail withTS2339("Property 'labels' does not exist").npx tsc --noEmit -p core/tsconfig.jsonis clean (0 errors) with the cast gone — that is the proof the assertion was never doing anything.On top of that,
preprocessRequest's label stripping had zero behavioural coverage (core/test/models/google_llm_test.tscontained no occurrence oflabels,preprocess, orapiBackend). Two new cases were added to the existingdescribe('generateContentAsync', ...)block, reusing the file's existingvi.mock('@google/genai')+TestGeminiharness. No existing test was modified, repurposed, skipped, or deleted.should clear labels on the request config for the Gemini API backend— asserts both the captured outgoing argument (callArg.config?.labels) and the caller's own object (llmRequest.config?.labels) areundefined. These pin two distinct contracts: what goes on the wire, and the documented in-place mutation of the caller's request.should preserve labels on the request config for the Vertex AI backend— the negative case. Without it, a mutation that clearedlabelsunconditionally would still pass test 1.The test's request literal (
config: {labels: {'adk-agent-name': 'agent'}}) only compiles becauseGenerateContentConfig.labelsis a declared property, so the tests double as a compile-time assertion of the same premise.One deliberate deviation from the plan, disclosed: the plan sketched these against the streaming path with
vi.fn().mockResolvedValue([]). A barevi.fn()is typedMock<(...args: any[]) => any>, which makes the capturedcallArgimplicitlyany— exactly the weak typing this PR exists to remove. Instead the mock is declared asvi.fn<typeof llm.apiClient.models.generateContent>(), socallArgis a realGenerateContentParametersandmockResolvedValue(new GenerateContentResponse())is a real SDK value. That required the non-streaming call (generateContentreturns a plainPromise<GenerateContentResponse>; the streaming sibling returns aPromise<AsyncGenerator<...>>that has no well-typed empty fixture). This tests the identical code path:preprocessRequestruns atgoogle_llm.ts:166, before any stream/non-stream branching, and the object handed to the SDK is the samellmRequest.configreference in both branches. No streaming-specific behaviour is involved in label stripping, so a second streaming copy would add coverage of nothing.Proof the tests can fail (mutation testing). Reverting the cast removal alone cannot fail a runtime test — the cast is type-level only, which is why the compiler is cited as the signal for that hunk. The two mutations below target the behaviour the new tests actually pin. The source was restored after each.
Mutation A — delete
llmRequest.config.labels = undefined;entirely. Test 1 FAILS, test 2 still passes:Mutation B — hoist the assignment above the
if (this.apiBackend === GoogleLLMVariant.GEMINI_API)guard. Test 2 FAILS, test 1 still passes:Commands run locally on the pushed commit (targeted only, never the full suite):
npm run ts:check(roottsc --noEmit, which also sweeps the test trees) has 280 pre-existing errors on an untouched checkout of this base. That count is identical before and after this change; the only textual difference is a line-number shift on one pre-existing unrelated error ingoogle_llm_test.ts(TS2820on a'audio'literal), caused by the added lines. No new diagnostic of any kind, and specifically noTS2339,TS2412, orTS2322.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
None, and none are warranted — stated explicitly rather than claiming a run that did not happen. The change is provably behaviour-preserving (a type-level assertion removed from a single assignment),
preprocessRequestisprivatewith no exported surface, and the affected path only executes against a live Google AI Studio API key. The compiler check plus the two unit tests above are the complete verification story. For the same reason notests/integrationortests/e2efile was added: that tree is reserved for build/packaging fixtures that runnpm installper fixture, and a fixture here would be cost with no signal.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.