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 <noreply@anthropic.com>
2026-11-06 20:58:14 -04:00 · 2026-06-26 11:37:39 +02:00
parent 73059a266b
commit ea573c25aa
8 changed files with 182 additions and 78 deletions
@@ -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<void> {

    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 <project> or set GEMINI_VERTEX_PROJECT/GOOGLE_CLOUD_PROJECT/GCLOUD_PROJECT/GOOGLE_PROJECT_ID.')
      process.exit(1)
@@ -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<void> {
  }

  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.')
@@ -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 ??
-      '<set GEMINI_VERTEX_PROJECT>'
+      getGeminiVertexProjectId(process.env) ?? '<set GEMINI_VERTEX_PROJECT>'
    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<string, string | boolean> {
    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 ??
@@ -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')
  })
@@ -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
  }
@@ -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<string, string | undefined>()
+
+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()
+})
@@ -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) {
@@ -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)}`