mirror of
https://github.com/Gitlawb/openclaude.git
synced 2026-11-06 20:58:14 -04:00
fix(gemini-vertex): tighten ADC completeness, neutral project placeholder, hermetic tests
Addresses CodeRabbit review on 6236de2 (5 findings):
- hasCompleteProviderSelection: use the *effective* auth mode
(resolveGeminiVertexAuthMode) so a GEMINI_ACCESS_TOKEN alongside a
GOOGLE_APPLICATION_CREDENTIALS file (which resolves to access-token and still
needs a project) is no longer treated as complete. The credentials file only
counts when the effective mode stays ADC. +2 regressions (token+creds, ADC).
- StartupScreen / buildCurrentProviderSummary: show a neutral
"<ADC-resolved project>" placeholder when an explicit ADC credential is
present (the client derives credential.projectId at runtime) instead of the
imperative "<set GEMINI_VERTEX_PROJECT>"; keep the actionable hint for
access-token/ambient setups. +startup regression.
- client.test.ts: add an ADC-only routing regression asserting the generated
Vertex URL uses the credential-discovered project (no env/profile project).
- Test isolation: snapshot a structuredClone of the global config before
mutating it (StartupScreen), and save/restore DISABLE_ERROR_REPORTING +
essential-traffic env so the logError gate control case is hermetic in CI.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
6236de2500
commit
365cc2e13b
@@ -318,12 +318,19 @@ export function buildCurrentProviderSummary(options?: {
|
|||||||
const vertexMetadata = getProviderPresetUiMetadata('gemini-vertex', vertexEnv)
|
const vertexMetadata = getProviderPresetUiMetadata('gemini-vertex', vertexEnv)
|
||||||
const location = getGeminiVertexLocation(vertexEnv)
|
const location = getGeminiVertexLocation(vertexEnv)
|
||||||
const project = getGeminiVertexProjectId(vertexEnv)
|
const project = getGeminiVertexProjectId(vertexEnv)
|
||||||
// Mirror the startup screen: the native client always targets
|
// Mirror the startup screen: an explicit ADC credential lets the native
|
||||||
// /projects/<project>/locations/<location> and requires a project, so
|
// client derive the project at runtime (credential.projectId), so show a
|
||||||
// surface a project-required state rather than a project-less endpoint.
|
// neutral placeholder there; access-token/ambient setups genuinely require
|
||||||
const endpoint = project
|
// a project, so keep the actionable hint rather than a project-less endpoint.
|
||||||
? `https://aiplatform.googleapis.com/v1/projects/${project}/locations/${location}`
|
const adcWillResolveProject =
|
||||||
: `https://aiplatform.googleapis.com/v1/projects/<set GEMINI_VERTEX_PROJECT>/locations/${location}`
|
vertexEnv.GEMINI_VERTEX_AUTH_MODE?.trim().toLowerCase() === 'adc' ||
|
||||||
|
(vertexEnv.GOOGLE_APPLICATION_CREDENTIALS?.trim().length ?? 0) > 0
|
||||||
|
const projectSegment =
|
||||||
|
project ??
|
||||||
|
(adcWillResolveProject
|
||||||
|
? '<ADC-resolved project>'
|
||||||
|
: '<set GEMINI_VERTEX_PROJECT>')
|
||||||
|
const endpoint = `https://aiplatform.googleapis.com/v1/projects/${projectSegment}/locations/${location}`
|
||||||
return {
|
return {
|
||||||
providerLabel: vertexMetadata.label,
|
providerLabel: vertexMetadata.label,
|
||||||
modelLabel: getSafeDisplayValue(
|
modelLabel: getSafeDisplayValue(
|
||||||
|
|||||||
@@ -50,9 +50,11 @@ const ENV_KEYS = [
|
|||||||
'GEMINI_VERTEX_MODEL',
|
'GEMINI_VERTEX_MODEL',
|
||||||
'GEMINI_VERTEX_PROJECT',
|
'GEMINI_VERTEX_PROJECT',
|
||||||
'GEMINI_VERTEX_LOCATION',
|
'GEMINI_VERTEX_LOCATION',
|
||||||
|
'GEMINI_VERTEX_AUTH_MODE',
|
||||||
'GOOGLE_CLOUD_PROJECT',
|
'GOOGLE_CLOUD_PROJECT',
|
||||||
'GCLOUD_PROJECT',
|
'GCLOUD_PROJECT',
|
||||||
'GOOGLE_PROJECT_ID',
|
'GOOGLE_PROJECT_ID',
|
||||||
|
'GOOGLE_APPLICATION_CREDENTIALS',
|
||||||
'MISTRAL_MODEL',
|
'MISTRAL_MODEL',
|
||||||
'ANTHROPIC_MODEL',
|
'ANTHROPIC_MODEL',
|
||||||
'CLAUDE_MODEL',
|
'CLAUDE_MODEL',
|
||||||
@@ -383,6 +385,19 @@ describe('detectProvider — Gemini Vertex endpoint reflects the runtime project
|
|||||||
expect(result.baseUrl).toContain('GEMINI_VERTEX_PROJECT')
|
expect(result.baseUrl).toContain('GEMINI_VERTEX_PROJECT')
|
||||||
})
|
})
|
||||||
|
|
||||||
|
test('shows a neutral placeholder for ADC setups that derive the project at runtime', () => {
|
||||||
|
process.env.CLAUDE_CODE_USE_GEMINI_VERTEX = '1'
|
||||||
|
// ADC credential present, no explicit project: getAnthropicClient resolves
|
||||||
|
// the project from credential.projectId at runtime, so the display must not
|
||||||
|
// imperatively demand GEMINI_VERTEX_PROJECT.
|
||||||
|
process.env.GOOGLE_APPLICATION_CREDENTIALS = '/tmp/adc.json'
|
||||||
|
|
||||||
|
const result = detectProvider()
|
||||||
|
expect(result.name).toBe('Gemini Vertex')
|
||||||
|
expect(result.baseUrl).toContain('ADC-resolved project')
|
||||||
|
expect(result.baseUrl).not.toContain('GEMINI_VERTEX_PROJECT')
|
||||||
|
})
|
||||||
|
|
||||||
test('detects Vertex for isEnvTruthy flag values (yes/on), not just 1/true', () => {
|
test('detects Vertex for isEnvTruthy flag values (yes/on), not just 1/true', () => {
|
||||||
process.env.CLAUDE_CODE_USE_GEMINI_VERTEX = 'yes'
|
process.env.CLAUDE_CODE_USE_GEMINI_VERTEX = 'yes'
|
||||||
process.env.GEMINI_VERTEX_PROJECT = 'my-project'
|
process.env.GEMINI_VERTEX_PROJECT = 'my-project'
|
||||||
@@ -409,7 +424,9 @@ describe('detectProvider — Gemini Vertex endpoint reflects the runtime project
|
|||||||
// active profile, exactly like getAnthropicClient. The profile's project
|
// active profile, exactly like getAnthropicClient. The profile's project
|
||||||
// (stored in baseUrl) and model must drive the startup display instead of
|
// (stored in baseUrl) and model must drive the startup display instead of
|
||||||
// falling through to the default Anthropic/OpenAI branch.
|
// falling through to the default Anthropic/OpenAI branch.
|
||||||
const restore = getGlobalConfig()
|
// Snapshot an immutable clone so the finally block fully resets the config
|
||||||
|
// instead of restoring a reference that saveGlobalConfig may have mutated.
|
||||||
|
const restore = structuredClone(getGlobalConfig())
|
||||||
saveGlobalConfig(current => ({
|
saveGlobalConfig(current => ({
|
||||||
...current,
|
...current,
|
||||||
providerProfiles: [
|
providerProfiles: [
|
||||||
|
|||||||
@@ -127,13 +127,21 @@ export function detectProvider(modelOverride?: string): { name: string; model: s
|
|||||||
DEFAULT_GEMINI_VERTEX_MODEL
|
DEFAULT_GEMINI_VERTEX_MODEL
|
||||||
const location = getGeminiVertexLocation(process.env)
|
const location = getGeminiVertexLocation(process.env)
|
||||||
const project = profileProject ?? getGeminiVertexProjectId(process.env)
|
const project = profileProject ?? getGeminiVertexProjectId(process.env)
|
||||||
// The native client always targets /projects/<project>/locations/<location>
|
// The native client always targets /projects/<project>/locations/<location>.
|
||||||
// and throws when no project resolves. Mirror that contract here: when a
|
// With an explicit ADC credential it can derive the project at runtime
|
||||||
// project is missing, surface a clear "project required" state instead of a
|
// (credential.projectId), so a missing project is not necessarily an error
|
||||||
// project-less endpoint that the runtime would never actually call.
|
// there — show a neutral placeholder. For access-token/ambient setups the
|
||||||
const baseUrl = project
|
// client throws without a project, so keep the actionable "set the project"
|
||||||
? `https://aiplatform.googleapis.com/v1/projects/${project}/locations/${location}`
|
// hint instead of a project-less endpoint the runtime would never call.
|
||||||
: `https://aiplatform.googleapis.com/v1/projects/<set GEMINI_VERTEX_PROJECT>/locations/${location}`
|
const adcWillResolveProject =
|
||||||
|
process.env.GEMINI_VERTEX_AUTH_MODE?.trim().toLowerCase() === 'adc' ||
|
||||||
|
(process.env.GOOGLE_APPLICATION_CREDENTIALS?.trim().length ?? 0) > 0
|
||||||
|
const projectSegment =
|
||||||
|
project ??
|
||||||
|
(adcWillResolveProject
|
||||||
|
? '<ADC-resolved project>'
|
||||||
|
: '<set GEMINI_VERTEX_PROJECT>')
|
||||||
|
const baseUrl = `https://aiplatform.googleapis.com/v1/projects/${projectSegment}/locations/${location}`
|
||||||
return { name: 'Gemini Vertex', model, baseUrl, isLocal: false }
|
return { name: 'Gemini Vertex', model, baseUrl, isLocal: false }
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ import {
|
|||||||
registerGateway,
|
registerGateway,
|
||||||
} from '../../integrations/index.js'
|
} from '../../integrations/index.js'
|
||||||
import * as providerProfilesModule from '../../utils/providerProfiles.js'
|
import * as providerProfilesModule from '../../utils/providerProfiles.js'
|
||||||
|
import * as geminiAuthModule from '../../utils/geminiAuth.js'
|
||||||
import { getAnthropicClient } from './client.js'
|
import { getAnthropicClient } from './client.js'
|
||||||
|
|
||||||
type FetchType = typeof globalThis.fetch
|
type FetchType = typeof globalThis.fetch
|
||||||
@@ -553,6 +554,64 @@ test('saved-profile-only Vertex prefers the profile project/model over ambient e
|
|||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|
||||||
|
test('ADC-only Vertex routing uses the credential-discovered project', async () => {
|
||||||
|
let capturedUrl: string | undefined
|
||||||
|
|
||||||
|
delete process.env.CLAUDE_CODE_USE_OPENAI
|
||||||
|
delete process.env.CLAUDE_CODE_USE_GEMINI
|
||||||
|
delete process.env.CLAUDE_CODE_USE_VERTEX
|
||||||
|
process.env.CLAUDE_CODE_USE_GEMINI_VERTEX = '1'
|
||||||
|
process.env.GEMINI_VERTEX_AUTH_MODE = 'adc'
|
||||||
|
process.env.GEMINI_VERTEX_LOCATION = 'us-central1'
|
||||||
|
process.env.GEMINI_VERTEX_MODEL = 'gemini-2.5-flash'
|
||||||
|
// No GEMINI_VERTEX_PROJECT / alias and no token: the project must come from
|
||||||
|
// the ADC credential (getAnthropicClient falls back to credential.projectId).
|
||||||
|
|
||||||
|
const spy = spyOn(
|
||||||
|
geminiAuthModule,
|
||||||
|
'resolveGeminiCredential',
|
||||||
|
).mockResolvedValue({
|
||||||
|
kind: 'adc',
|
||||||
|
credential: 'adc-access-token',
|
||||||
|
projectId: 'adc-discovered-project',
|
||||||
|
} as Awaited<ReturnType<typeof geminiAuthModule.resolveGeminiCredential>>)
|
||||||
|
|
||||||
|
globalThis.fetch = (async (input, _init) => {
|
||||||
|
capturedUrl =
|
||||||
|
typeof input === 'string'
|
||||||
|
? input
|
||||||
|
: input instanceof URL
|
||||||
|
? input.toString()
|
||||||
|
: input.url
|
||||||
|
return new Response(
|
||||||
|
JSON.stringify({
|
||||||
|
candidates: [{ content: { parts: [{ text: 'vertex ok' }] } }],
|
||||||
|
}),
|
||||||
|
{ headers: { 'Content-Type': 'application/json' } },
|
||||||
|
)
|
||||||
|
}) as FetchType
|
||||||
|
|
||||||
|
try {
|
||||||
|
const client = (await getAnthropicClient({
|
||||||
|
maxRetries: 0,
|
||||||
|
model: 'gemini-2.5-flash',
|
||||||
|
})) as unknown as ShimClient
|
||||||
|
|
||||||
|
await client.beta.messages.create({
|
||||||
|
model: 'gemini-2.5-flash',
|
||||||
|
messages: [{ role: 'user', content: 'hello' }],
|
||||||
|
max_tokens: 64,
|
||||||
|
stream: false,
|
||||||
|
})
|
||||||
|
|
||||||
|
expect(capturedUrl).toBe(
|
||||||
|
'https://us-central1-aiplatform.googleapis.com/v1/projects/adc-discovered-project/locations/us-central1/publishers/google/models/gemini-2.5-flash:generateContent',
|
||||||
|
)
|
||||||
|
} finally {
|
||||||
|
spy.mockRestore()
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
test('Gemini Vertex provider uses GEMINI_VERTEX_MODEL instead of generic OPENAI_MODEL', async () => {
|
test('Gemini Vertex provider uses GEMINI_VERTEX_MODEL instead of generic OPENAI_MODEL', async () => {
|
||||||
let capturedUrl: string | undefined
|
let capturedUrl: string | undefined
|
||||||
|
|
||||||
|
|||||||
@@ -7,7 +7,10 @@ import { getInMemoryErrors, logError } from './log.js'
|
|||||||
// saved active profile (effective-provider check), so a saved-profile-only
|
// saved active profile (effective-provider check), so a saved-profile-only
|
||||||
// Vertex session does not leak errors to the in-memory/reporting sink.
|
// Vertex session does not leak errors to the in-memory/reporting sink.
|
||||||
describe('logError error-reporting gate — Gemini Vertex effective provider', () => {
|
describe('logError error-reporting gate — Gemini Vertex effective provider', () => {
|
||||||
const PROVIDER_FLAGS = [
|
// Provider-selection flags plus every other env input the logError() gate
|
||||||
|
// reads (DISABLE_ERROR_REPORTING and the essential-traffic switch), so the
|
||||||
|
// control case is hermetic and does not depend on leaked CI state.
|
||||||
|
const GATING_ENV = [
|
||||||
'CLAUDE_CODE_USE_GEMINI_VERTEX',
|
'CLAUDE_CODE_USE_GEMINI_VERTEX',
|
||||||
'CLAUDE_CODE_USE_BEDROCK',
|
'CLAUDE_CODE_USE_BEDROCK',
|
||||||
'CLAUDE_CODE_USE_VERTEX',
|
'CLAUDE_CODE_USE_VERTEX',
|
||||||
@@ -16,25 +19,28 @@ describe('logError error-reporting gate — Gemini Vertex effective provider', (
|
|||||||
'CLAUDE_CODE_USE_GEMINI',
|
'CLAUDE_CODE_USE_GEMINI',
|
||||||
'CLAUDE_CODE_USE_MISTRAL',
|
'CLAUDE_CODE_USE_MISTRAL',
|
||||||
'CLAUDE_CODE_USE_GITHUB',
|
'CLAUDE_CODE_USE_GITHUB',
|
||||||
|
'DISABLE_ERROR_REPORTING',
|
||||||
|
'CLAUDE_CODE_DISABLE_NONESSENTIAL_TRAFFIC',
|
||||||
|
'DISABLE_TELEMETRY',
|
||||||
]
|
]
|
||||||
const savedFlags: Record<string, string | undefined> = {}
|
const savedEnv: Record<string, string | undefined> = {}
|
||||||
let restoreConfig: ReturnType<typeof getGlobalConfig>
|
let restoreConfig: ReturnType<typeof getGlobalConfig>
|
||||||
|
|
||||||
beforeEach(() => {
|
beforeEach(() => {
|
||||||
for (const flag of PROVIDER_FLAGS) {
|
for (const key of GATING_ENV) {
|
||||||
savedFlags[flag] = process.env[flag]
|
savedEnv[key] = process.env[key]
|
||||||
delete process.env[flag]
|
delete process.env[key]
|
||||||
}
|
}
|
||||||
restoreConfig = getGlobalConfig()
|
restoreConfig = structuredClone(getGlobalConfig())
|
||||||
})
|
})
|
||||||
|
|
||||||
afterEach(() => {
|
afterEach(() => {
|
||||||
saveGlobalConfig(() => restoreConfig)
|
saveGlobalConfig(() => restoreConfig)
|
||||||
for (const flag of PROVIDER_FLAGS) {
|
for (const key of GATING_ENV) {
|
||||||
if (savedFlags[flag] === undefined) {
|
if (savedEnv[key] === undefined) {
|
||||||
delete process.env[flag]
|
delete process.env[key]
|
||||||
} else {
|
} else {
|
||||||
process.env[flag] = savedFlags[flag]
|
process.env[key] = savedEnv[key]
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|||||||
@@ -1507,6 +1507,68 @@ describe('applyActiveProviderProfileFromConfig', () => {
|
|||||||
}
|
}
|
||||||
})
|
})
|
||||||
|
|
||||||
|
test('applies saved profile when a Vertex token coexists with a credentials file but no project', async () => {
|
||||||
|
// A GEMINI_ACCESS_TOKEN makes the effective auth mode access-token even when
|
||||||
|
// GOOGLE_APPLICATION_CREDENTIALS is also set, so the credentials file must
|
||||||
|
// NOT make this token-only (no-project) selection look complete — the
|
||||||
|
// native client would still throw "project is required".
|
||||||
|
const { applyActiveProviderProfileFromConfig } =
|
||||||
|
await importFreshProviderProfileModules()
|
||||||
|
process.env.CLAUDE_CODE_USE_GEMINI_VERTEX = '1'
|
||||||
|
process.env.GEMINI_ACCESS_TOKEN = 'ya29.startup-token'
|
||||||
|
process.env.GOOGLE_APPLICATION_CREDENTIALS = '/tmp/adc.json'
|
||||||
|
try {
|
||||||
|
const applied = applyActiveProviderProfileFromConfig({
|
||||||
|
providerProfiles: [
|
||||||
|
buildProfile({
|
||||||
|
id: 'saved_openai',
|
||||||
|
baseUrl: 'https://api.openai.com/v1',
|
||||||
|
model: 'gpt-4o',
|
||||||
|
}),
|
||||||
|
],
|
||||||
|
activeProviderProfileId: 'saved_openai',
|
||||||
|
} as any)
|
||||||
|
|
||||||
|
expect(applied?.id).toBe('saved_openai')
|
||||||
|
expect(process.env.OPENAI_MODEL).toBe('gpt-4o')
|
||||||
|
} finally {
|
||||||
|
delete process.env.CLAUDE_CODE_USE_GEMINI_VERTEX
|
||||||
|
delete process.env.GEMINI_ACCESS_TOKEN
|
||||||
|
delete process.env.GOOGLE_APPLICATION_CREDENTIALS
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
|
test('keeps a Vertex ADC credentials-file startup selection (no project needed)', async () => {
|
||||||
|
// A credentials file with the effective mode still ADC (no token) can derive
|
||||||
|
// the project at runtime, so it is a complete selection and must not be
|
||||||
|
// overridden by the saved profile.
|
||||||
|
const { applyActiveProviderProfileFromConfig } =
|
||||||
|
await importFreshProviderProfileModules()
|
||||||
|
process.env.CLAUDE_CODE_USE_GEMINI_VERTEX = '1'
|
||||||
|
process.env.GOOGLE_APPLICATION_CREDENTIALS = '/tmp/adc.json'
|
||||||
|
try {
|
||||||
|
const applied = applyActiveProviderProfileFromConfig({
|
||||||
|
providerProfiles: [
|
||||||
|
buildProfile({
|
||||||
|
id: 'saved_openai',
|
||||||
|
baseUrl: 'https://api.openai.com/v1',
|
||||||
|
model: 'gpt-4o',
|
||||||
|
}),
|
||||||
|
],
|
||||||
|
activeProviderProfileId: 'saved_openai',
|
||||||
|
} as any)
|
||||||
|
|
||||||
|
expect(applied).toBeUndefined()
|
||||||
|
expect(process.env.CLAUDE_CODE_USE_GEMINI_VERTEX).toBe('1')
|
||||||
|
expect(process.env.GOOGLE_APPLICATION_CREDENTIALS).toBe('/tmp/adc.json')
|
||||||
|
expect(process.env.CLAUDE_CODE_USE_OPENAI).toBeUndefined()
|
||||||
|
expect(process.env.OPENAI_MODEL).toBeUndefined()
|
||||||
|
} finally {
|
||||||
|
delete process.env.CLAUDE_CODE_USE_GEMINI_VERTEX
|
||||||
|
delete process.env.GOOGLE_APPLICATION_CREDENTIALS
|
||||||
|
}
|
||||||
|
})
|
||||||
|
|
||||||
test('keeps Gemini Vertex access-token startup selection when a project is set', async () => {
|
test('keeps Gemini Vertex access-token startup selection when a project is set', async () => {
|
||||||
// With a project alias the access-token selection is usable, so it stays
|
// With a project alias the access-token selection is usable, so it stays
|
||||||
// and the saved profile must not hijack it.
|
// and the saved profile must not hijack it.
|
||||||
|
|||||||
@@ -512,13 +512,16 @@ function hasCompleteProviderSelection(
|
|||||||
}
|
}
|
||||||
// ADC is the only auth mode that can derive a project from the credentials
|
// ADC is the only auth mode that can derive a project from the credentials
|
||||||
// (getAnthropicClient falls back to credential.projectId for ADC only), so
|
// (getAnthropicClient falls back to credential.projectId for ADC only), so
|
||||||
// an ADC selection is complete even without an explicit project alias.
|
// an ADC selection is complete even without an explicit project alias. Use
|
||||||
const authMode = trimOrUndefined(
|
// the *effective* mode (resolveGeminiVertexAuthMode), not the raw env: a
|
||||||
|
// GEMINI_ACCESS_TOKEN makes the runtime resolve access-token even when a
|
||||||
|
// credentials file is also present, and that path still needs a project.
|
||||||
|
const explicitAuthMode = trimOrUndefined(
|
||||||
processEnv.GEMINI_VERTEX_AUTH_MODE,
|
processEnv.GEMINI_VERTEX_AUTH_MODE,
|
||||||
)?.toLowerCase()
|
)?.toLowerCase()
|
||||||
if (authMode === 'adc') return true
|
if (explicitAuthMode === 'adc') return true
|
||||||
if (
|
if (
|
||||||
authMode !== 'access-token' &&
|
resolveGeminiVertexAuthMode(processEnv) === 'adc' &&
|
||||||
trimOrUndefined(processEnv.GOOGLE_APPLICATION_CREDENTIALS) !== undefined
|
trimOrUndefined(processEnv.GOOGLE_APPLICATION_CREDENTIALS) !== undefined
|
||||||
) {
|
) {
|
||||||
return true
|
return true
|
||||||
|
|||||||
Reference in New Issue
Block a user