From e2e12c92010fcbc13f4daecec9aa6638b4c70bb7 Mon Sep 17 00:00:00 2001 From: gitdevine <210788251+gitdevine@users.noreply.github.com> Date: Sun, 14 Jun 2026 19:39:17 +0200 Subject: [PATCH] fix(gemini-vertex): address CodeRabbit review on the routing PR - providerProfile.ts: honor an explicit shell GEMINI_VERTEX_AUTH_MODE over the persisted/default value (same precedence as model/project). - providerProfiles.ts: validate the active Vertex project in the profile alignment check so a project drift forces re-apply instead of routing to the wrong project (only when the profile pins a project; ADC-resolved stays free). - messages.ts: harden isNotEmptyMessage against malformed user/assistant slots ({ type } / { message: {} }) that previously crashed on message.message.content; add a regression test. - providerConfig.local.test.ts: clear a possibly-leaked CLAUDE_CODE_USE_OPENAI so the Gemini Vertex cache-scope assertion can't pass/fail for the wrong reason. The Critical "typecheck must pass" item is a false positive: both `bun run typecheck` and `bun run typecheck:type-tests` pass cleanly. Co-Authored-By: Claude Fable 5 --- src/services/api/providerConfig.local.test.ts | 7 ++++ src/utils/messages.isNotEmptyMessage.test.ts | 32 +++++++++++++++++++ src/utils/messages.ts | 8 +++++ src/utils/providerProfile.ts | 11 ++++--- src/utils/providerProfiles.ts | 15 ++++++++- 5 files changed, 68 insertions(+), 5 deletions(-) create mode 100644 src/utils/messages.isNotEmptyMessage.test.ts diff --git a/src/services/api/providerConfig.local.test.ts b/src/services/api/providerConfig.local.test.ts index 0357684d3..a90f5275e 100644 --- a/src/services/api/providerConfig.local.test.ts +++ b/src/services/api/providerConfig.local.test.ts @@ -330,11 +330,18 @@ test('does not classify Gemini Vertex sessions as first-party bootstrap traffic' // Maintainer P2: with only the Gemini Vertex flag set, the cache scope // must be null (third-party), not 'firstParty' — otherwise bootstrap // options are fetched through the Anthropic first-party path. + const priorUseOpenAI = process.env.CLAUDE_CODE_USE_OPENAI + // A leaked CLAUDE_CODE_USE_OPENAI from the runner env would take the OpenAI + // branch and make this pass/fail for the wrong reason. + delete process.env.CLAUDE_CODE_USE_OPENAI process.env.CLAUDE_CODE_USE_GEMINI_VERTEX = '1' try { expect(getAdditionalModelOptionsCacheScope()).toBeNull() } finally { delete process.env.CLAUDE_CODE_USE_GEMINI_VERTEX + if (priorUseOpenAI !== undefined) { + process.env.CLAUDE_CODE_USE_OPENAI = priorUseOpenAI + } } }) diff --git a/src/utils/messages.isNotEmptyMessage.test.ts b/src/utils/messages.isNotEmptyMessage.test.ts new file mode 100644 index 000000000..6681818ec --- /dev/null +++ b/src/utils/messages.isNotEmptyMessage.test.ts @@ -0,0 +1,32 @@ +import { describe, expect, test } from 'bun:test' +import { isNotEmptyMessage } from './messages.js' +import type { Message } from '../types/message.js' + +describe('isNotEmptyMessage malformed-history hardening', () => { + test('treats falsy / non-object slots as empty', () => { + expect(isNotEmptyMessage(undefined as unknown as Message)).toBe(false) + expect(isNotEmptyMessage(null as unknown as Message)).toBe(false) + expect(isNotEmptyMessage('oops' as unknown as Message)).toBe(false) + }) + + test('does not crash on malformed user/assistant slots', () => { + // These passed the object guard but have no message.content and used to + // crash when it was read. + expect(isNotEmptyMessage({ type: 'assistant' } as unknown as Message)).toBe(false) + expect( + isNotEmptyMessage({ type: 'user', message: {} } as unknown as Message), + ).toBe(false) + expect( + isNotEmptyMessage({ type: 'user', message: { content: null } } as unknown as Message), + ).toBe(false) + }) + + test('still reports a non-empty string user message', () => { + expect( + isNotEmptyMessage({ + type: 'user', + message: { content: 'hello' }, + } as unknown as Message), + ).toBe(true) + }) +}) diff --git a/src/utils/messages.ts b/src/utils/messages.ts index 345077085..78b0117f8 100644 --- a/src/utils/messages.ts +++ b/src/utils/messages.ts @@ -718,6 +718,14 @@ export function isNotEmptyMessage(message: Message): boolean { return true } + // Defensive: user/assistant slots can be malformed (e.g. `{ type: 'user' }` + // or `{ type: 'user', message: {} }`) after an error path; reading + // message.message.content below would then crash. Treat them as empty. + const inner = (message as { message?: { content?: unknown } }).message + if (!inner || typeof inner !== 'object' || inner.content == null) { + return false + } + if (typeof message.message.content === 'string') { return message.message.content.trim().length > 0 } diff --git a/src/utils/providerProfile.ts b/src/utils/providerProfile.ts index 7eb1099fd..15f6d96ac 100644 --- a/src/utils/providerProfile.ts +++ b/src/utils/providerProfile.ts @@ -1613,12 +1613,15 @@ export async function buildLaunchEnv(options: { } if (selectedProfile === 'gemini-vertex') { + // Shell value wins over the persisted one (same precedence as model/project + // below) so an explicit GEMINI_VERTEX_AUTH_MODE isn't silently replaced. + const isVertexAuthMode = (value: string | undefined) => + value === 'access-token' || value === 'adc' ? value : undefined const env = buildGeminiVertexProfileEnv({ authMode: - persistedEnv.GEMINI_VERTEX_AUTH_MODE === 'access-token' || - persistedEnv.GEMINI_VERTEX_AUTH_MODE === 'adc' - ? persistedEnv.GEMINI_VERTEX_AUTH_MODE - : 'adc', + isVertexAuthMode(processEnv.GEMINI_VERTEX_AUTH_MODE) ?? + isVertexAuthMode(persistedEnv.GEMINI_VERTEX_AUTH_MODE) ?? + 'adc', model: processEnv.GEMINI_VERTEX_MODEL || persistedEnv.GEMINI_VERTEX_MODEL, project: processEnv.GEMINI_VERTEX_PROJECT || diff --git a/src/utils/providerProfiles.ts b/src/utils/providerProfiles.ts index a912ff858..452e4788b 100644 --- a/src/utils/providerProfiles.ts +++ b/src/utils/providerProfiles.ts @@ -624,6 +624,18 @@ function isProcessEnvAlignedWithProfile( } if (compatibilityMode === 'gemini-vertex') { + // The profile stores its Vertex project in baseUrl. When it pins one, treat + // the env as aligned only if the active project still matches — otherwise a + // project drift would skip re-apply and route to the wrong project. When the + // profile has no explicit project (ADC resolves it), don't constrain it. + const activeProject = + trimOrUndefined(processEnv.GEMINI_VERTEX_PROJECT) ?? + trimOrUndefined(processEnv.GOOGLE_CLOUD_PROJECT) ?? + trimOrUndefined(processEnv.GCLOUD_PROJECT) ?? + trimOrUndefined(processEnv.GOOGLE_PROJECT_ID) + const projectAligned = profile.baseUrl + ? sameOptionalEnvValue(activeProject, profile.baseUrl) + : true return ( processEnv.CLAUDE_CODE_USE_GEMINI_VERTEX !== undefined && processEnv.CLAUDE_CODE_USE_OPENAI === undefined && @@ -635,7 +647,8 @@ function isProcessEnvAlignedWithProfile( processEnv.CLAUDE_CODE_USE_FOUNDRY === undefined && processEnv.OPENAI_BASE_URL === undefined && processEnv.OPENAI_MODEL === undefined && - sameOptionalEnvValue(processEnv.GEMINI_VERTEX_MODEL, getPrimaryModel(profile.model)) + sameOptionalEnvValue(processEnv.GEMINI_VERTEX_MODEL, getPrimaryModel(profile.model)) && + projectAligned ) }