From cb162e7164d3e603d7a152ab7d2a960b91653546 Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Fri, 22 May 2026 23:19:17 +0800 Subject: [PATCH] =?UTF-8?q?fix(telemetry):=20R5=20review=20fixups=20?= =?UTF-8?q?=E2=80=94=20Vertex=20destination=20+=20["*"]=20trim=20+=20docs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review pass on commit 1c8528a56 (host-scoped session-id header): 1. **Vertex AI destination guessing** (geminiContentGenerator/index.ts) `@google/genai` routes to `{region}-aiplatform.googleapis.com` (not `generativelanguage.googleapis.com`) when `vertexai: true` and no `baseUrl`. The previous "guess generativelanguage" default would have mis-bucketed Vertex traffic under any operator-supplied allowlist that covered the public Gemini endpoint but not the Vertex one. Today invisible (both off the default allowlist), but a latent gotcha for operators tuning `telemetry.sessionIdHeaderHosts`. Fix: pass `undefined` when `config.baseUrl` is unset (fail-closed โ€” no header). Operators who want correlation against Google endpoints must set `baseUrl` explicitly, which is also the SDK's input for destination resolution. 2. **`["*"]` broadcast escape hatch tolerates whitespace** (llm-correlation-fetch.ts) `[" * "]` (a settings.json hand-edit with a stray space) previously silently fell back to "no host matches" โ€” the opposite of operator intent. Now `.trim()` before comparing, so common whitespace mistakes still trigger broadcast. 3. **Doc note on wrap-time allowlist snapshot** (llm-correlation-fetch.ts JSDoc) The session id is read live per-request, but `trustedHosts` is snapshotted once at `wrapFetchWithCorrelation` call time. Spell this out in the JSDoc so a future maintainer doesn't read the live `getSessionId()` and assume the allowlist is the same shape. 4. **Defensive test coverage** (trusted-llm-hosts.test.ts, llm-correlation-fetch.test.ts) Added: extractRequestHost with explicit port / userinfo / query / fragment / IPv6 bracket form. Whitespace `[" * "]` broadcast test. IPv6 case documents the "bracketed โ†’ never matches" behavior is intentional fail-closed for the named-host allowlist scope. ๐Ÿค– Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) --- .../core/geminiContentGenerator/index.test.ts | 18 +++++++----- .../src/core/geminiContentGenerator/index.ts | 21 +++++++------- .../telemetry/llm-correlation-fetch.test.ts | 19 ++++++++++++ .../src/telemetry/llm-correlation-fetch.ts | 15 ++++++++-- .../src/telemetry/trusted-llm-hosts.test.ts | 29 +++++++++++++++++++ 5 files changed, 83 insertions(+), 19 deletions(-) diff --git a/packages/core/src/core/geminiContentGenerator/index.test.ts b/packages/core/src/core/geminiContentGenerator/index.test.ts index c343de686f..d9d9f92350 100644 --- a/packages/core/src/core/geminiContentGenerator/index.test.ts +++ b/packages/core/src/core/geminiContentGenerator/index.test.ts @@ -161,17 +161,21 @@ describe('createGeminiContentGenerator', () => { ); }); - it('includes X-Qwen-Code-Session-Id when allowlist override covers googleapis.com', () => { - // Operator opts back in for Google's endpoint via settings override. + it('omits X-Qwen-Code-Session-Id when baseUrl is unset, even with allowlist override (factory fail-closed for unknown destination)', () => { + // The factory only passes a destinationUrl to staticCorrelationHeaders + // when `config.baseUrl` is set, because the Gemini SDK has two + // invisible defaults (generativelanguage.googleapis.com for public, + // {region}-aiplatform.googleapis.com for Vertex). Guessing one would + // mis-bucket the other under a googleapis-covering allowlist override. + // Operators who want correlation against Google endpoints must set + // `baseUrl` explicitly. mockConfig = { getUsageStatisticsEnabled: vi.fn().mockReturnValue(false), getContentGeneratorConfig: vi.fn().mockReturnValue({}), getCliVersion: vi.fn().mockReturnValue('1.0.0'), getTelemetryEnabled: vi.fn().mockReturnValue(true), getSessionId: vi.fn().mockReturnValue('sess-gemini'), - getTelemetrySessionIdHeaderHosts: vi - .fn() - .mockReturnValue(['*.googleapis.com']), + getTelemetrySessionIdHeaderHosts: vi.fn().mockReturnValue(['*']), } as unknown as Config; const config = { model: 'gemini-1.5-flash', @@ -182,8 +186,8 @@ describe('createGeminiContentGenerator', () => { const callArgs = vi.mocked(GeminiContentGenerator).mock.calls[0]?.[0] as { httpOptions: { headers: Record }; }; - expect(callArgs.httpOptions.headers['X-Qwen-Code-Session-Id']).toBe( - 'sess-gemini', + expect(callArgs.httpOptions.headers).not.toHaveProperty( + 'X-Qwen-Code-Session-Id', ); }); }); diff --git a/packages/core/src/core/geminiContentGenerator/index.ts b/packages/core/src/core/geminiContentGenerator/index.ts index 41896a3cd8..3d0f86ef8a 100644 --- a/packages/core/src/core/geminiContentGenerator/index.ts +++ b/packages/core/src/core/geminiContentGenerator/index.ts @@ -46,18 +46,19 @@ export function createGeminiContentGenerator( // SDK's cached headers retain the OLD session id until the contentGenerator // is recreated. See design doc ยง8.6 + #4384 follow-up sub-issue tracking. // - // Destination passed in is what the host-allowlist check uses. With the - // default `DEFAULT_SESSION_ID_HEADER_HOSTS` (Alibaba/DashScope-only), - // Google's default endpoint `generativelanguage.googleapis.com` is NOT - // on the list, so the header is naturally omitted for vanilla Gemini API - // calls โ€” matching the "first-party only" scope. Operators who deliberately - // want correlation against a Google endpoint can add it via - // `telemetry.sessionIdHeaderHosts` in settings. - const destinationUrl = - config.baseUrl ?? 'https://generativelanguage.googleapis.com'; + // Destination resolution: only pass when we KNOW the destination โ€” i.e. + // when the operator has explicitly set `config.baseUrl`. The SDK has two + // different invisible defaults (`generativelanguage.googleapis.com` for + // public Gemini, `{region}-aiplatform.googleapis.com` for Vertex AI) and + // guessing wrong here would mis-bucket Vertex traffic if the operator + // overrides `telemetry.sessionIdHeaderHosts` to include any googleapis + // domain. Passing `undefined` makes `staticCorrelationHeaders` return + // `{}` (no header) โ€” the fail-closed default. Operators who want + // correlation against a Google endpoint set `baseUrl` explicitly + // (which is the same input the SDK uses to resolve the destination). headers = { ...headers, - ...staticCorrelationHeaders(gcConfig, destinationUrl), + ...staticCorrelationHeaders(gcConfig, config.baseUrl), }; const httpOptions = config.baseUrl ? { diff --git a/packages/core/src/telemetry/llm-correlation-fetch.test.ts b/packages/core/src/telemetry/llm-correlation-fetch.test.ts index aaca69751b..19a6e332e5 100644 --- a/packages/core/src/telemetry/llm-correlation-fetch.test.ts +++ b/packages/core/src/telemetry/llm-correlation-fetch.test.ts @@ -498,6 +498,25 @@ describe('wrapFetchWithCorrelation โ€” host allowlist gating', () => { ); }); + it('tolerates whitespace around "*" so [" * "] still enables broadcast', async () => { + // Defensive: settings.json hand-edits are easy to get a stray space + // wrong. Without trim() the operator would silently get "no host + // matches" instead of broadcast. + const m = makeFetchMock(); + const wrapped = wrapFetchWithCorrelation( + m.fetch, + mockConfig({ + enabled: true, + sessionId: 'sess-A', + hosts: [' * '], + }), + ); + await wrapped('https://api.openai.com/v1/chat/completions'); + expect((m.lastInit()?.headers as Headers).get(SESSION_ID_HEADER)).toBe( + 'sess-A', + ); + }); + it('respects [] override to fully disable injection', async () => { const m = makeFetchMock(); const wrapped = wrapFetchWithCorrelation( diff --git a/packages/core/src/telemetry/llm-correlation-fetch.ts b/packages/core/src/telemetry/llm-correlation-fetch.ts index a858d5527c..4fc7921564 100644 --- a/packages/core/src/telemetry/llm-correlation-fetch.ts +++ b/packages/core/src/telemetry/llm-correlation-fetch.ts @@ -70,6 +70,15 @@ type FetchLikeLoose = ( * Reading `config.getSessionId()` from inside the wrapper on every call * gives the live value. * + * Note on `trustedHosts`: snapshotted once at wrap time, not read per + * request. The session id is live-read but the allowlist is not โ€” a + * mid-session change to `telemetry.sessionIdHeaderHosts` in settings.json + * takes effect at next content-generator init (any change that mutates + * Config snapshot the wrapper retains is by definition a Config-level + * concern, not a request-time concern). Operators tuning the scope live + * should restart, or call the openai/anthropic clients via a fresh + * provider after settings reload. + * * The caller is responsible for choosing the base fetch โ€” usually * `runtimeOptions?.fetch ?? globalThis.fetch` so proxy-aware fetch (set up * by `buildRuntimeFetchOptions`) is preserved when ProxyAgent is in use. @@ -102,7 +111,9 @@ export function wrapFetchWithCorrelation( // Wildcard escape hatch so operators who want the old broadcast // behavior can opt in via `["*"]` without us extending the pattern // grammar in `matchesTrustedHost` (which would tempt other globbing). - const broadcastAll = trustedHosts.some((p) => p === '*'); + // `.trim()` so `[" * "]` (whitespace from settings.json hand-edit) still + // triggers broadcast rather than silently degrading to "no host matches". + const broadcastAll = trustedHosts.some((p) => p.trim() === '*'); const wrapped: FetchLikeLoose = async function correlationFetch( input: string | URL | Request, @@ -185,7 +196,7 @@ export function staticCorrelationHeaders( const trustedHosts = config.getTelemetrySessionIdHeaderHosts?.() ?? DEFAULT_SESSION_ID_HEADER_HOSTS; - const broadcastAll = trustedHosts.some((p) => p === '*'); + const broadcastAll = trustedHosts.some((p) => p.trim() === '*'); if (!broadcastAll) { let host: string; try { diff --git a/packages/core/src/telemetry/trusted-llm-hosts.test.ts b/packages/core/src/telemetry/trusted-llm-hosts.test.ts index 248625c680..06fb8dd15d 100644 --- a/packages/core/src/telemetry/trusted-llm-hosts.test.ts +++ b/packages/core/src/telemetry/trusted-llm-hosts.test.ts @@ -99,6 +99,35 @@ describe('extractRequestHost', () => { ).toBe('api.anthropic.com'); }); + it('strips explicit port (URL.hostname does not include port)', () => { + expect(extractRequestHost('https://dashscope.aliyuncs.com:443/v1')).toBe( + 'dashscope.aliyuncs.com', + ); + }); + + it('strips userinfo (URL.hostname does not include user/password)', () => { + expect( + extractRequestHost('https://user:pw@dashscope.aliyuncs.com/v1'), + ).toBe('dashscope.aliyuncs.com'); + }); + + it('ignores query string and fragment', () => { + expect( + extractRequestHost('https://dashscope.aliyuncs.com/v1?key=secret#frag'), + ).toBe('dashscope.aliyuncs.com'); + }); + + it('returns bracketed form for IPv6 (will never match a string pattern, behaves as fail-closed)', () => { + // Document the IPv6 behavior so it's intentional rather than a + // surprise: URL.hostname returns `[::1]` (with brackets), our + // pattern grammar has no `[โ€ฆ]` form, so IPv6 destinations are + // effectively never on the allowlist. This is acceptable because + // qwen-code's allowlist is for named first-party endpoints, not + // raw IPs. If/when raw-IP correlation becomes a real ask, extend + // matchesTrustedHost โ€” don't change extractRequestHost. + expect(extractRequestHost('http://[::1]:8080/x')).toBe('[::1]'); + }); + it('returns undefined for unparseable string', () => { expect(extractRequestHost('not a url')).toBeUndefined(); });