Skip to content

Commit 6b0e582

Browse files
committed
fix(vertex): bind parsed credentials before truthiness check + extract shared util
Four review items from the PR review round: - Precedence bug at gemini.ts:225-233 and anthropic-vertex.ts:43-50: the truthiness check was on the raw vertexJsonCredentials field, so path-shaped input (which parseVertexJsonCredentials correctly returns undefined for) still entered the JSON branch with credentials: undefined instead of falling through to vertexKeyFile. Bind parseVertexJsonCredentials's result first and branch on it. - Extract parseVertexJsonCredentials from gemini.ts to a new utils/vertex-credentials.ts module. anthropic-vertex.ts previously imported from ./gemini, which pulled in the @google/genai runtime dependency unnecessarily. The shared util has no provider-specific imports. - a11y on the Vertex.tsx warning: added role="alert" and aria-live="polite" so screen readers announce the dynamic warning when it appears. - Mixed-input regression tests: added cases asserting that when vertexJsonCredentials is path-shaped AND vertexKeyFile is set, the GoogleAuth/GoogleGenAI client is constructed with the keyFile, not the (now undefined) credentials. Locks in the fallback contract. The pre-existing path-shape-only tests were also updated to match the corrected fallthrough: with no vertexKeyFile, neither GoogleAuth (anthropic-vertex) nor googleAuthOptions (gemini) is constructed.
1 parent a77360d commit 6b0e582

5 files changed

Lines changed: 111 additions & 47 deletions

File tree

src/api/providers/__tests__/vertex-credentials.spec.ts

Lines changed: 61 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,10 @@ vitest.mock("@anthropic-ai/vertex-sdk", () => ({
4545
})),
4646
}))
4747

48-
import { GeminiHandler, parseVertexJsonCredentials } from "../gemini"
48+
import { GeminiHandler } from "../gemini"
4949
import { VertexHandler } from "../vertex"
5050
import { AnthropicVertexHandler } from "../anthropic-vertex"
51+
import { parseVertexJsonCredentials } from "../utils/vertex-credentials"
5152

5253
const VALID_CREDS_JSON = JSON.stringify({
5354
type: "service_account",
@@ -155,7 +156,7 @@ describe("GeminiHandler vertex credentials wiring", () => {
155156
expect(args.googleAuthOptions.credentials).toMatchObject({ type: "service_account" })
156157
})
157158

158-
it("warns and passes undefined credentials when the field looks like a path", () => {
159+
it("warns and falls through past the JSON branch when the field looks like a path", () => {
159160
new GeminiHandler({
160161
apiModelId: "gemini-2.0-flash-001",
161162
vertexProjectId: "p",
@@ -167,7 +168,34 @@ describe("GeminiHandler vertex credentials wiring", () => {
167168
expect(warnSpy).toHaveBeenCalledTimes(1)
168169
expect(googleGenAICtor).toHaveBeenCalledTimes(1)
169170
const args = googleGenAICtor.mock.calls[0][0]
170-
expect(args.googleAuthOptions.credentials).toBeUndefined()
171+
// With only a path-shaped vertexJsonCredentials (no vertexKeyFile),
172+
// the ternary falls all the way through to the bare isVertex branch:
173+
// new GoogleGenAI({ vertexai: true, project, location }) — no
174+
// googleAuthOptions block.
175+
expect(args.googleAuthOptions).toBeUndefined()
176+
expect(args.vertexai).toBe(true)
177+
// Generic "Error parsing JSON" must not fire for the path case.
178+
expect(errorSpy).not.toHaveBeenCalled()
179+
})
180+
181+
it("uses vertexKeyFile when vertexJsonCredentials is path-shaped AND vertexKeyFile is set", () => {
182+
new GeminiHandler({
183+
apiModelId: "gemini-2.0-flash-001",
184+
vertexProjectId: "p",
185+
vertexRegion: "us-central1",
186+
vertexJsonCredentials: "C:\\Users\\dev\\sa.json",
187+
vertexKeyFile: "my-key-file.json",
188+
isVertex: true,
189+
})
190+
191+
expect(googleGenAICtor).toHaveBeenCalledTimes(1)
192+
const args = googleGenAICtor.mock.calls[0][0]
193+
// The path-shaped input must not poison the JSON branch; the fallback
194+
// to the vertexKeyFile branch must take effect.
195+
expect(args.googleAuthOptions?.keyFile).toBe("my-key-file.json")
196+
expect(args.googleAuthOptions?.credentials).toBeUndefined()
197+
// The warning for the path-shaped input still fires.
198+
expect(warnSpy).toHaveBeenCalledTimes(1)
171199
// Generic "Error parsing JSON" must not fire for the path case.
172200
expect(errorSpy).not.toHaveBeenCalled()
173201
})
@@ -200,7 +228,7 @@ describe("VertexHandler inherits the path-shape guard from GeminiHandler", () =>
200228
warnSpy.mockRestore()
201229
})
202230

203-
it("warns and passes undefined credentials when the field looks like a POSIX path", () => {
231+
it("warns and falls through past the JSON branch when the field looks like a POSIX path", () => {
204232
new VertexHandler({
205233
apiModelId: "gemini-2.0-flash-001",
206234
vertexProjectId: "p",
@@ -210,7 +238,11 @@ describe("VertexHandler inherits the path-shape guard from GeminiHandler", () =>
210238

211239
expect(warnSpy).toHaveBeenCalledTimes(1)
212240
const args = googleGenAICtor.mock.calls[0][0]
213-
expect(args.googleAuthOptions.credentials).toBeUndefined()
241+
// VertexHandler extends GeminiHandler with isVertex:true. With only a
242+
// path-shaped vertexJsonCredentials, the ternary falls through to the
243+
// bare isVertex branch with no googleAuthOptions.
244+
expect(args.googleAuthOptions).toBeUndefined()
245+
expect(args.vertexai).toBe(true)
214246
})
215247
})
216248

@@ -243,7 +275,7 @@ describe("AnthropicVertexHandler vertex credentials wiring", () => {
243275
expect(args.credentials).toMatchObject({ type: "service_account" })
244276
})
245277

246-
it("warns and passes undefined credentials when the field looks like a Windows path", () => {
278+
it("warns and skips the GoogleAuth construction when the field looks like a Windows path", () => {
247279
new AnthropicVertexHandler({
248280
apiModelId: "claude-3-5-sonnet-v2@20241022",
249281
vertexProjectId: "p",
@@ -252,9 +284,32 @@ describe("AnthropicVertexHandler vertex credentials wiring", () => {
252284
})
253285

254286
expect(warnSpy).toHaveBeenCalledTimes(1)
287+
// With only a path-shaped vertexJsonCredentials (no vertexKeyFile),
288+
// every branch in the constructor falls through to the bare
289+
// `new AnthropicVertex({ projectId, region })` and GoogleAuth is
290+
// never instantiated.
291+
expect(googleAuthCtor).not.toHaveBeenCalled()
292+
expect(errorSpy).not.toHaveBeenCalled()
293+
})
294+
295+
it("uses vertexKeyFile when vertexJsonCredentials is path-shaped AND vertexKeyFile is set", () => {
296+
new AnthropicVertexHandler({
297+
apiModelId: "claude-3-5-sonnet-v2@20241022",
298+
vertexProjectId: "p",
299+
vertexRegion: "us-east5",
300+
vertexJsonCredentials: "C:\\Users\\dev\\sa.json",
301+
vertexKeyFile: "my-key-file.json",
302+
})
303+
304+
// The path-shaped input must not poison the JSON branch; the fallback
305+
// to the vertexKeyFile branch must take effect.
255306
expect(googleAuthCtor).toHaveBeenCalledTimes(1)
256307
const args = googleAuthCtor.mock.calls[0][0]
308+
expect(args.keyFile).toBe("my-key-file.json")
257309
expect(args.credentials).toBeUndefined()
310+
// The warning for the path-shaped input still fires.
311+
expect(warnSpy).toHaveBeenCalledTimes(1)
312+
// Generic "Error parsing JSON" must not fire for the path case.
258313
expect(errorSpy).not.toHaveBeenCalled()
259314
})
260315

src/api/providers/anthropic-vertex.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ import {
2323
} from "../../core/prompts/tools/native-tools/converters"
2424

2525
import { BaseProvider } from "./base-provider"
26-
import { parseVertexJsonCredentials } from "./gemini"
26+
import { parseVertexJsonCredentials } from "./utils/vertex-credentials"
2727
import type { SingleCompletionHandler, ApiHandlerCreateMessageMetadata } from "../index"
2828

2929
// https://docs.anthropic.com/en/api/claude-on-vertex-ai
@@ -40,13 +40,15 @@ export class AnthropicVertexHandler extends BaseProvider implements SingleComple
4040
const projectId = this.options.vertexProjectId ?? "not-provided"
4141
const region = this.options.vertexRegion ?? "us-east5"
4242

43-
if (this.options.vertexJsonCredentials) {
43+
const parsedVertexCredentials = parseVertexJsonCredentials(this.options.vertexJsonCredentials)
44+
45+
if (parsedVertexCredentials) {
4446
this.client = new AnthropicVertex({
4547
projectId,
4648
region,
4749
googleAuth: new GoogleAuth({
4850
scopes: ["https://www.googleapis.com/auth/cloud-platform"],
49-
credentials: parseVertexJsonCredentials(this.options.vertexJsonCredentials),
51+
credentials: parsedVertexCredentials,
5052
}),
5153
})
5254
} else if (this.options.vertexKeyFile) {

src/api/providers/gemini.ts

Lines changed: 5 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -7,16 +7,13 @@ import {
77
type GroundingMetadata,
88
FunctionCallingConfigMode,
99
} from "@google/genai"
10-
import type { JWTInput } from "google-auth-library"
11-
1210
import {
1311
type ModelInfo,
1412
type GeminiModelId,
1513
geminiDefaultModelId,
1614
geminiModels,
1715
ApiProviderError,
1816
} from "@roo-code/types"
19-
import { safeJsonParse } from "@roo-code/core"
2017
import { TelemetryService } from "@roo-code/telemetry"
2118

2219
import type { ApiHandlerOptions } from "../../shared/api"
@@ -28,43 +25,12 @@ import { getModelParams } from "../transform/model-params"
2825

2926
import type { SingleCompletionHandler, ApiHandlerCreateMessageMetadata } from "../index"
3027
import { BaseProvider } from "./base-provider"
28+
import { parseVertexJsonCredentials } from "./utils/vertex-credentials"
3129

3230
type GeminiHandlerOptions = ApiHandlerOptions & {
3331
isVertex?: boolean
3432
}
3533

36-
// Detects when the "Google Cloud Credentials" field has received a filesystem
37-
// path instead of the raw JSON contents of a service-account key file. Users
38-
// often confuse this with GOOGLE_APPLICATION_CREDENTIALS (which IS a path),
39-
// and the sibling "Google Cloud Key File Path" field is where a path belongs.
40-
// Returns the parsed credentials object when the input looks like JSON, or
41-
// undefined when the field is empty, path-shaped, or unparseable.
42-
export function parseVertexJsonCredentials(value: string | undefined): JWTInput | undefined {
43-
const trimmed = value?.trim()
44-
if (!trimmed) {
45-
return undefined
46-
}
47-
48-
const looksLikePath =
49-
/^[A-Za-z]:[\\/]/.test(trimmed) || // Windows: C:\... or C:/...
50-
trimmed.startsWith("/") || // POSIX absolute: /home/...
51-
trimmed.startsWith("~") || // POSIX home: ~/...
52-
trimmed.startsWith(".") // POSIX relative: ./... or ../...
53-
54-
if (looksLikePath) {
55-
const preview = trimmed.length > 40 ? `${trimmed.slice(0, 40)}…` : trimmed
56-
console.warn(
57-
`[Vertex] The 'Google Cloud Credentials' field appears to contain a file path ("${preview}"), ` +
58-
"but this field expects the raw JSON contents of a service-account key file. " +
59-
"If you have a path to the credentials file, paste it into the 'Google Cloud Key File Path' field instead, " +
60-
"or leave both fields empty and use the GOOGLE_APPLICATION_CREDENTIALS environment variable.",
61-
)
62-
return undefined
63-
}
64-
65-
return safeJsonParse<JWTInput>(trimmed, undefined, "Vertex credentials")
66-
}
67-
6834
// Gemini documents function declaration schemas as a selected OpenAPI-style
6935
// subset with single-value `type` plus `nullable`. In practice, third-party
7036
// MCP schemas often include broader JSON Schema metadata/composition that has
@@ -222,13 +188,15 @@ export class GeminiHandler extends BaseProvider implements SingleCompletionHandl
222188
const location = this.options.vertexRegion ?? "not-provided"
223189
const apiKey = this.options.geminiApiKey ?? "not-provided"
224190

225-
this.client = this.options.vertexJsonCredentials
191+
const parsedVertexCredentials = parseVertexJsonCredentials(this.options.vertexJsonCredentials)
192+
193+
this.client = parsedVertexCredentials
226194
? new GoogleGenAI({
227195
vertexai: true,
228196
project,
229197
location,
230198
googleAuthOptions: {
231-
credentials: parseVertexJsonCredentials(this.options.vertexJsonCredentials),
199+
credentials: parsedVertexCredentials,
232200
},
233201
})
234202
: this.options.vertexKeyFile
Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
import type { JWTInput } from "google-auth-library"
2+
3+
import { safeJsonParse } from "@roo-code/core"
4+
5+
// Detects when the "Google Cloud Credentials" field has received a filesystem
6+
// path instead of the raw JSON contents of a service-account key file. Users
7+
// often confuse this with GOOGLE_APPLICATION_CREDENTIALS (which IS a path),
8+
// and the sibling "Google Cloud Key File Path" field is where a path belongs.
9+
// Returns the parsed credentials object when the input looks like JSON, or
10+
// undefined when the field is empty, path-shaped, or unparseable.
11+
export function parseVertexJsonCredentials(value: string | undefined): JWTInput | undefined {
12+
const trimmed = value?.trim()
13+
if (!trimmed) {
14+
return undefined
15+
}
16+
17+
const looksLikePath =
18+
/^[A-Za-z]:[\\/]/.test(trimmed) || // Windows: C:\... or C:/...
19+
trimmed.startsWith("/") || // POSIX absolute: /home/...
20+
trimmed.startsWith("~") || // POSIX home: ~/...
21+
trimmed.startsWith(".") // POSIX relative: ./... or ../...
22+
23+
if (looksLikePath) {
24+
const preview = trimmed.length > 40 ? `${trimmed.slice(0, 40)}…` : trimmed
25+
console.warn(
26+
`[Vertex] The 'Google Cloud Credentials' field appears to contain a file path ("${preview}"), ` +
27+
"but this field expects the raw JSON contents of a service-account key file. " +
28+
"If you have a path to the credentials file, paste it into the 'Google Cloud Key File Path' field instead, " +
29+
"or leave both fields empty and use the GOOGLE_APPLICATION_CREDENTIALS environment variable.",
30+
)
31+
return undefined
32+
}
33+
34+
return safeJsonParse<JWTInput>(trimmed, undefined, "Vertex credentials")
35+
}

webview-ui/src/components/settings/providers/Vertex.tsx

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,7 +92,11 @@ export const Vertex = ({ apiConfiguration, setApiConfigurationField }: VertexPro
9292
<label className="block font-medium mb-1">{t("settings:providers.googleCloudCredentials")}</label>
9393
</VSCodeTextField>
9494
{credentialsLooksLikePath && (
95-
<div data-testid="vertex-credentials-path-warning" className="text-sm text-vscode-errorForeground">
95+
<div
96+
data-testid="vertex-credentials-path-warning"
97+
role="alert"
98+
aria-live="polite"
99+
className="text-sm text-vscode-errorForeground">
96100
<Trans
97101
i18nKey="settings:providers.googleCloudCredentialsPathWarning"
98102
components={{

0 commit comments

Comments
 (0)