From ea573c25aab95f183496c0a738eb7bb06dae0d13 Mon Sep 17 00:00:00 2001 From: gitdevine <210788251+gitdevine@users.noreply.github.com> Date: Tue, 16 Jun 2026 01:49:08 +0200 Subject: [PATCH] fix(gemini-vertex): address CodeRabbit review on PR-2 (8 findings) - client.ts: re-resolve the Vertex credential per request via getAccessToken so long-lived clients pick up refreshed ADC tokens instead of a stale one captured at construction (Critical); coerce geminiVertexModel to a plain string so the model fallback satisfies tsc and never passes undefined. - toolUseIds.ts: coerce non-string ids before .replace so loosely typed history blocks (number/object/undefined) cannot throw on the wire path. - system-check.ts / provider-launch.ts / provider-bootstrap.ts: resolve the Gemini Vertex project through the shared getGeminiVertexProjectId resolver instead of three drifting OR chains, matching the runtime validator/client contract and adding whitespace normalization. - providerSelectionFlags.test.ts: keep the parity scan non-tautological by excluding the registry and its own test. - bugfixes.test.ts: add a sectionBetween() helper that asserts both anchors resolve before slicing the drift-guard regions (a missing end anchor used to silently widen the slice and pass); fix the StartupScreen detectProvider end anchor it exposed. - deprecation.test.ts: lock the gemini-vertex bypass so Anthropic model retirement dates never leak into the Vertex deprecation banner. Co-Authored-By: Claude Fable 5 --- scripts/provider-bootstrap.ts | 11 ++-- scripts/provider-launch.ts | 12 ++-- scripts/system-check.ts | 40 ++++++------ src/__tests__/bugfixes.test.ts | 77 +++++++++++++++--------- src/services/api/client.ts | 36 ++++++++--- src/utils/model/deprecation.test.ts | 62 +++++++++++++++++++ src/utils/providerSelectionFlags.test.ts | 11 +++- src/utils/toolUseIds.ts | 11 ++-- 8 files changed, 182 insertions(+), 78 deletions(-) create mode 100644 src/utils/model/deprecation.test.ts diff --git a/scripts/provider-bootstrap.ts b/scripts/provider-bootstrap.ts index 5e41e1b6e..c70764645 100644 --- a/scripts/provider-bootstrap.ts +++ b/scripts/provider-bootstrap.ts @@ -29,6 +29,7 @@ import { listAtomicChatModels, listOllamaModels, } from './provider-discovery.ts' +import { getGeminiVertexProjectId } from '../src/utils/geminiAuth.ts' function parseArg(name: string): string | null { const args = process.argv.slice(2) @@ -94,13 +95,9 @@ async function main(): Promise { env = builtEnv } else if (selected === 'gemini-vertex') { - const project = - argBaseUrl || - process.env.GEMINI_VERTEX_PROJECT || - process.env.GOOGLE_CLOUD_PROJECT || - process.env.GCLOUD_PROJECT || - process.env.GOOGLE_PROJECT_ID || - null + // Reuse the runtime project resolver (same normalization as the provider + // validator) rather than an ad-hoc OR chain. + const project = argBaseUrl?.trim() || getGeminiVertexProjectId(process.env) || null if (!project) { console.error('Gemini Vertex profile requires a project. Use --base-url or set GEMINI_VERTEX_PROJECT/GOOGLE_CLOUD_PROJECT/GCLOUD_PROJECT/GOOGLE_PROJECT_ID.') process.exit(1) diff --git a/scripts/provider-launch.ts b/scripts/provider-launch.ts index 17e00d1ae..227737c39 100644 --- a/scripts/provider-launch.ts +++ b/scripts/provider-launch.ts @@ -15,6 +15,7 @@ import { type ProfileFile, type ProviderProfile, } from '../src/utils/providerProfile.ts' +import { getGeminiVertexProjectId } from '../src/utils/geminiAuth.ts' import { getAtomicChatChatBaseUrl, getOllamaChatBaseUrl, @@ -232,14 +233,11 @@ async function main(): Promise { } if (profile === 'gemini-vertex') { - const hasProject = Boolean( - env.GEMINI_VERTEX_PROJECT || - env.GOOGLE_CLOUD_PROJECT || - env.GCLOUD_PROJECT || - env.GOOGLE_PROJECT_ID, - ) + // Reuse the runtime project resolver so launch validation matches the + // provider validator / client contract instead of drifting. + const hasProject = Boolean(getGeminiVertexProjectId(env)) const hasCredential = Boolean( - env.GEMINI_ACCESS_TOKEN || env.GOOGLE_APPLICATION_CREDENTIALS, + env.GEMINI_ACCESS_TOKEN?.trim() || env.GOOGLE_APPLICATION_CREDENTIALS?.trim(), ) if (!hasProject || !hasCredential) { console.error('Gemini Vertex requires a project (GEMINI_VERTEX_PROJECT/GOOGLE_CLOUD_PROJECT/GCLOUD_PROJECT/GOOGLE_PROJECT_ID) and a credential (GEMINI_ACCESS_TOKEN or Google ADC via GOOGLE_APPLICATION_CREDENTIALS). Save a gemini-vertex profile with `/provider` or set those env vars.') diff --git a/scripts/system-check.ts b/scripts/system-check.ts index f16364d32..740560f4e 100644 --- a/scripts/system-check.ts +++ b/scripts/system-check.ts @@ -26,6 +26,7 @@ import { redactSecretSubstringsForDisplay, type SecretValueSource, } from '../src/utils/providerSecrets.js' +import { getGeminiVertexProjectId } from '../src/utils/geminiAuth.js' import { redactUrlForDisplay } from '../src/utils/urlRedaction.js' import { MIN_NODE_ENGINE_RANGE, @@ -342,11 +343,7 @@ function currentBaseUrl(): string { if (isTruthy(process.env.CLAUDE_CODE_USE_GEMINI_VERTEX)) { const location = process.env.GEMINI_VERTEX_LOCATION ?? 'global' const project = - process.env.GEMINI_VERTEX_PROJECT ?? - process.env.GOOGLE_CLOUD_PROJECT ?? - process.env.GCLOUD_PROJECT ?? - process.env.GOOGLE_PROJECT_ID ?? - '' + getGeminiVertexProjectId(process.env) ?? '' return `https://aiplatform.googleapis.com/v1/projects/${project}/locations/${location}` } if (isTruthy(process.env.CLAUDE_CODE_USE_GEMINI)) { @@ -390,16 +387,20 @@ function checkGeminiVertexEnv(): CheckResult[] { const results: CheckResult[] = [] results.push(pass('Provider mode', 'Google Vertex AI Gemini provider enabled.')) - const model = process.env.GEMINI_VERTEX_MODEL - const project = - process.env.GEMINI_VERTEX_PROJECT ?? - process.env.GOOGLE_CLOUD_PROJECT ?? - process.env.GCLOUD_PROJECT ?? - process.env.GOOGLE_PROJECT_ID - const location = process.env.GEMINI_VERTEX_LOCATION ?? 'global' + const nonBlank = (v: string | undefined): string | undefined => { + const t = v?.trim() + return t ? t : undefined + } + const model = nonBlank(process.env.GEMINI_VERTEX_MODEL) + // Same resolver as the runtime validator / client so doctor never reports a + // different project than the one the provider would actually use. + const project = getGeminiVertexProjectId(process.env) + const location = nonBlank(process.env.GEMINI_VERTEX_LOCATION) ?? 'global' + const accessToken = nonBlank(process.env.GEMINI_ACCESS_TOKEN) + const adcFile = nonBlank(process.env.GOOGLE_APPLICATION_CREDENTIALS) const authMode = - process.env.GEMINI_VERTEX_AUTH_MODE ?? - (process.env.GEMINI_ACCESS_TOKEN ? 'access-token' : 'adc') + nonBlank(process.env.GEMINI_VERTEX_AUTH_MODE) ?? + (accessToken ? 'access-token' : 'adc') if (!project) { results.push( @@ -421,13 +422,13 @@ function checkGeminiVertexEnv(): CheckResult[] { if (authMode === 'access-token') { results.push( - process.env.GEMINI_ACCESS_TOKEN + accessToken ? pass('GEMINI_VERTEX_AUTH', 'access-token configured.') : fail('GEMINI_VERTEX_AUTH', 'access-token mode selected but GEMINI_ACCESS_TOKEN is missing.'), ) } else { results.push( - process.env.GOOGLE_APPLICATION_CREDENTIALS + adcFile ? pass('GEMINI_VERTEX_AUTH', 'ADC via GOOGLE_APPLICATION_CREDENTIALS.') : pass('GEMINI_VERTEX_AUTH', 'ADC mode — relying on ambient Google Application Default Credentials.'), ) @@ -876,12 +877,7 @@ export function serializeSafeEnvSummary(): Record { return { CLAUDE_CODE_USE_GEMINI_VERTEX: true, GEMINI_VERTEX_MODEL: process.env.GEMINI_VERTEX_MODEL ?? '(unset, default: gemini-2.5-flash)', - GEMINI_VERTEX_PROJECT: - process.env.GEMINI_VERTEX_PROJECT ?? - process.env.GOOGLE_CLOUD_PROJECT ?? - process.env.GCLOUD_PROJECT ?? - process.env.GOOGLE_PROJECT_ID ?? - '(unset)', + GEMINI_VERTEX_PROJECT: getGeminiVertexProjectId(process.env) ?? '(unset)', GEMINI_VERTEX_LOCATION: process.env.GEMINI_VERTEX_LOCATION ?? 'global', GEMINI_VERTEX_AUTH_MODE: process.env.GEMINI_VERTEX_AUTH_MODE ?? diff --git a/src/__tests__/bugfixes.test.ts b/src/__tests__/bugfixes.test.ts index ccaee233d..9b3cf3f54 100644 --- a/src/__tests__/bugfixes.test.ts +++ b/src/__tests__/bugfixes.test.ts @@ -20,6 +20,24 @@ import type { PluginHookMatcher } from '../utils/settings/types.js' const SRC = resolve(import.meta.dir, '..') const file = (relative: string) => Bun.file(resolve(SRC, relative)) +/** + * Slice the region of `content` delimited by two source anchors, asserting + * both anchors are present first. Without this, a renamed/removed anchor makes + * indexOf return -1 and slice() silently widen or mis-scope the section, so the + * drift guard could keep passing on an unrelated match elsewhere in the file. + */ +function sectionBetween( + content: string, + startAnchor: string, + endAnchor: string, +): string { + const start = content.indexOf(startAnchor) + expect(start, `start anchor not found: ${startAnchor}`).toBeGreaterThanOrEqual(0) + const end = content.indexOf(endAnchor, start) + expect(end, `end anchor not found after start: ${endAnchor}`).toBeGreaterThan(start) + return content.slice(start, end) +} + // --------------------------------------------------------------------------- // Fix 1: Gemini `store: false` rejection // --------------------------------------------------------------------------- @@ -514,26 +532,29 @@ describe('Context overflow 500 fix', () => { describe('Gemini Vertex provider drift guards', () => { test('API customer, model flag, and startup detection include Gemini Vertex', async () => { const authContent = await file('utils/auth.ts').text() - const apiCustomerStart = authContent.indexOf('export function is1PApiCustomer') - const apiCustomerEnd = authContent.indexOf('export function getOauthAccountInfo', apiCustomerStart) - const apiCustomerSection = authContent.slice(apiCustomerStart, apiCustomerEnd) + const apiCustomerSection = sectionBetween( + authContent, + 'export function is1PApiCustomer', + 'export function getOauthAccountInfo', + ) expect(apiCustomerSection).toContain('CLAUDE_CODE_USE_GEMINI_VERTEX') const providerFlagContent = await file('utils/providerFlag.ts').text() - const modelFlagStart = providerFlagContent.indexOf('export function applyModelFlagFromArgs') - const modelFlagEnd = providerFlagContent.indexOf('export function applyProviderFlag', modelFlagStart) - const modelFlagSection = providerFlagContent.slice(modelFlagStart, modelFlagEnd) + const modelFlagSection = sectionBetween( + providerFlagContent, + 'export function applyModelFlagFromArgs', + 'export function applyProviderFlag', + ) expect(modelFlagSection).toContain('CLAUDE_CODE_USE_GEMINI_VERTEX') expect(modelFlagSection).toContain('GEMINI_VERTEX_MODEL') const startupScreenContent = await file('components/StartupScreen.ts').text() - const detectProviderStart = startupScreenContent.indexOf('export function detectProvider') - const detectProviderEnd = startupScreenContent.indexOf('export function StartupScreen', detectProviderStart) - const detectProviderSection = startupScreenContent.slice( - detectProviderStart, - detectProviderEnd, + const detectProviderSection = sectionBetween( + startupScreenContent, + 'export function detectProvider', + 'function boxRow', ) expect(detectProviderSection).toContain('CLAUDE_CODE_USE_GEMINI_VERTEX') @@ -542,11 +563,10 @@ describe('Gemini Vertex provider drift guards', () => { test('log and GitHub onboarding clear Gemini Vertex provider state', async () => { const logContent = await file('utils/log.ts').text() - const errorReportingStart = logContent.indexOf('Cloud providers') - const errorReportingEnd = logContent.indexOf('process.env.DISABLE_ERROR_REPORTING', errorReportingStart) - const errorReportingSection = logContent.slice( - errorReportingStart, - errorReportingEnd, + const errorReportingSection = sectionBetween( + logContent, + 'Cloud providers', + 'process.env.DISABLE_ERROR_REPORTING', ) expect(errorReportingSection).toContain('CLAUDE_CODE_USE_GEMINI_VERTEX') @@ -554,11 +574,10 @@ describe('Gemini Vertex provider drift guards', () => { const onboardGithubContent = await file( 'commands/onboard-github/onboard-github.tsx', ).text() - const providerKeysStart = onboardGithubContent.indexOf('PROVIDER_SPECIFIC_KEYS') - const providerKeysEnd = onboardGithubContent.indexOf('])', providerKeysStart) - const providerKeysSection = onboardGithubContent.slice( - providerKeysStart, - providerKeysEnd, + const providerKeysSection = sectionBetween( + onboardGithubContent, + 'PROVIDER_SPECIFIC_KEYS', + '])', ) expect(providerKeysSection).toContain('CLAUDE_CODE_USE_GEMINI_VERTEX') @@ -566,18 +585,22 @@ describe('Gemini Vertex provider drift guards', () => { test('auth treats Gemini Vertex as a third-party provider', async () => { const content = await file('utils/auth.ts').text() - const is3PStart = content.indexOf('const is3P =') - const is3PEnd = content.indexOf('// Check if user has configured', is3PStart) - const is3PSection = content.slice(is3PStart, is3PEnd) + const is3PSection = sectionBetween( + content, + 'const is3P =', + '// Check if user has configured', + ) expect(is3PSection).toContain('CLAUDE_CODE_USE_GEMINI_VERTEX') }) test('API preconnect skips Gemini Vertex routing', async () => { const content = await file('utils/apiPreconnect.ts').text() - const cloudSkipStart = content.indexOf('// Skip if using a cloud provider') - const cloudSkipEnd = content.indexOf('// Skip if proxy/mTLS/unix', cloudSkipStart) - const cloudSkipSection = content.slice(cloudSkipStart, cloudSkipEnd) + const cloudSkipSection = sectionBetween( + content, + '// Skip if using a cloud provider', + '// Skip if proxy/mTLS/unix', + ) expect(cloudSkipSection).toContain('CLAUDE_CODE_USE_GEMINI_VERTEX') }) diff --git a/src/services/api/client.ts b/src/services/api/client.ts index a49085e79..d2a043d40 100644 --- a/src/services/api/client.ts +++ b/src/services/api/client.ts @@ -49,6 +49,7 @@ import { } from '../../integrations/routeMetadata.js' import { resolveOpenAIShimRuntimeContext } from '../../integrations/runtimeMetadata.js' import { + DEFAULT_GEMINI_VERTEX_MODEL, getGeminiVertexLocation, getGeminiVertexModel, getGeminiVertexProjectId, @@ -551,20 +552,29 @@ export async function getAnthropicClient({ : undefined const project = getGeminiVertexProjectId(process.env) ?? profileProject const location = getGeminiVertexLocation(process.env) + // getGeminiVertexModel already falls back to the default model; the trailing + // literal only satisfies its `string | undefined` signature (it never + // actually returns undefined) so geminiVertexModel stays a plain string. const geminiVertexModel = process.env.GEMINI_VERTEX_MODEL?.trim() || profileModel || model?.trim() || getGeminiVertexModel(process.env) || - 'gemini-3.5-flash' - const credential = await resolveGeminiCredential({ - ...process.env, - GEMINI_AUTH_MODE: - process.env.GEMINI_VERTEX_AUTH_MODE ?? - (process.env.GEMINI_ACCESS_TOKEN ? 'access-token' : 'adc'), - GEMINI_API_KEY: undefined, - GOOGLE_API_KEY: undefined, - } as NodeJS.ProcessEnv) + DEFAULT_GEMINI_VERTEX_MODEL + // Re-resolve on every call instead of capturing one token: ADC access + // tokens expire (~1h), so a long-lived client must pick up refreshed + // tokens. resolveGeminiCredential memoizes the ADC path (5-min TTL) and + // GoogleAuth refreshes internally, so this stays cheap. + const resolveVertexCredential = () => + resolveGeminiCredential({ + ...process.env, + GEMINI_AUTH_MODE: + process.env.GEMINI_VERTEX_AUTH_MODE ?? + (process.env.GEMINI_ACCESS_TOKEN ? 'access-token' : 'adc'), + GEMINI_API_KEY: undefined, + GOOGLE_API_KEY: undefined, + } as NodeJS.ProcessEnv) + const credential = await resolveVertexCredential() if (credential.kind === 'none') { throw new Error('Gemini Vertex authentication requires GEMINI_ACCESS_TOKEN or Google ADC credentials') } @@ -578,7 +588,13 @@ export async function getAnthropicClient({ project: vertexProject, location, model: geminiVertexModel, - getAccessToken: async () => credential.credential, + getAccessToken: async () => { + const fresh = await resolveVertexCredential() + if (fresh.kind === 'none') { + throw new Error('Gemini Vertex authentication is no longer available (token expired or revoked; re-set GEMINI_ACCESS_TOKEN or refresh ADC).') + } + return fresh.credential + }, ...(resolvedFetch ? { fetch: resolvedFetch } : {}), }) as unknown as Anthropic } diff --git a/src/utils/model/deprecation.test.ts b/src/utils/model/deprecation.test.ts new file mode 100644 index 000000000..335f444fe --- /dev/null +++ b/src/utils/model/deprecation.test.ts @@ -0,0 +1,62 @@ +import { afterEach, beforeEach, expect, test } from 'bun:test' + +import { + acquireSharedMutationLock, + releaseSharedMutationLock, +} from '../../test/sharedMutationLock.js' +import { getModelDeprecationWarning } from './deprecation.js' + +// Provider selection is driven entirely by env flags read live by +// getAPIProvider(); snapshot the ones this suite mutates so it stays hermetic. +const TOUCHED_ENV = [ + 'CLAUDE_CODE_USE_GEMINI_VERTEX', + 'CLAUDE_CODE_USE_GEMINI', + 'CLAUDE_CODE_USE_OPENAI', + 'CLAUDE_CODE_USE_GITHUB', + 'CLAUDE_CODE_USE_BEDROCK', + 'CLAUDE_CODE_USE_VERTEX', + 'CLAUDE_CODE_USE_FOUNDRY', + 'CLAUDE_CODE_USE_MISTRAL', +] as const + +const originalEnv = new Map() + +beforeEach(async () => { + await acquireSharedMutationLock('model/deprecation.test.ts') + for (const key of TOUCHED_ENV) { + originalEnv.set(key, process.env[key]) + delete process.env[key] + } +}) + +afterEach(() => { + try { + for (const key of TOUCHED_ENV) { + const value = originalEnv.get(key) + if (value === undefined) { + delete process.env[key] + } else { + process.env[key] = value + } + } + } finally { + releaseSharedMutationLock() + } +}) + +// claude-3-opus is deprecated under first-party / Anthropic-wire providers. +const DEPRECATED_FIRST_PARTY_MODEL = 'claude-3-opus-20240229' + +test('first-party provider still surfaces the deprecation warning', () => { + // Sanity anchor: the bypass below is only meaningful if this model is + // otherwise reported as deprecated. + const warning = getModelDeprecationWarning(DEPRECATED_FIRST_PARTY_MODEL) + expect(warning).toContain('Claude 3 Opus') +}) + +test('Gemini Vertex provider bypasses model deprecation warnings', () => { + process.env.CLAUDE_CODE_USE_GEMINI_VERTEX = '1' + // Gemini Vertex serves its own model catalog, so Anthropic retirement dates + // must never leak into its deprecation banner. + expect(getModelDeprecationWarning(DEPRECATED_FIRST_PARTY_MODEL)).toBeNull() +}) diff --git a/src/utils/providerSelectionFlags.test.ts b/src/utils/providerSelectionFlags.test.ts index 72d02a9c5..7f4a3a9b1 100644 --- a/src/utils/providerSelectionFlags.test.ts +++ b/src/utils/providerSelectionFlags.test.ts @@ -44,11 +44,20 @@ describe('PROVIDER_SELECTION_FLAGS parity', () => { expect(unregistered).toEqual([]) }) - test('registered flags actually appear in src', () => { + test('registered flags are actually consumed outside the registry definition', () => { const srcRoot = join(import.meta.dir, '..') const glob = new Bun.Glob('**/*.{ts,tsx}') let allContent = '' for (const relPath of glob.scanSync({ cwd: srcRoot })) { + // Exclude the registry itself (and its test) so the assertion proves the + // flag is referenced by real runtime code, not just by its own + // definition — otherwise the check is tautological. + if ( + relPath.endsWith('providerSelectionFlags.ts') || + relPath.endsWith('providerSelectionFlags.test.ts') + ) { + continue + } allContent += readFileSync(join(srcRoot, relPath), 'utf8') } for (const flag of PROVIDER_SELECTION_FLAGS) { diff --git a/src/utils/toolUseIds.ts b/src/utils/toolUseIds.ts index 39e0e30e0..5d77f3967 100644 --- a/src/utils/toolUseIds.ts +++ b/src/utils/toolUseIds.ts @@ -16,18 +16,21 @@ export const MAX_WIRE_TOOL_ID_LENGTH = 64 * distinct. Dependency-free so shims and message normalization can share it. */ export function sanitizeToolUseIdForWire( - value: string, + value: unknown, maxLength = MAX_WIRE_TOOL_ID_LENGTH, ): string { - const clean = value.replace(/[^A-Za-z0-9_-][\s\S]*$/, '') + // Callers pass ids from loosely typed history blocks; a non-string id + // (number/object/undefined) would throw on .replace below. Coerce first. + const str = typeof value === 'string' ? value : value == null ? '' : String(value) + const clean = str.replace(/[^A-Za-z0-9_-][\s\S]*$/, '') if (clean.length > 0 && clean.length <= maxLength) { return clean } // Deterministic non-cryptographic hash of the FULL original value (FNV-1a) // — keeps distinct ids distinct even when their clean prefixes collide. let hash = 0x811c9dc5 - for (let i = 0; i < value.length; i++) { - hash ^= value.charCodeAt(i) + for (let i = 0; i < str.length; i++) { + hash ^= str.charCodeAt(i) hash = Math.imul(hash, 0x01000193) >>> 0 } const tail = `_${hash.toString(36)}`