feat(telemetry): propagate X-Qwen-Code-Session-Id on outbound LLM requests (part 2 of #4384)#4393
Conversation
…uests Part 2 of #4384. Stacks on top of PR #4390 (traceparent via undici). Adds a product-namespaced HTTP header X-Qwen-Code-Session-Id to every outbound LLM request when telemetry is enabled, so server-side ingestion can correlate observed requests with qwen-code session metric/log records. Pattern matched from claude-code (X-Claude-Code-Session-Id, verified at src/services/api/client.ts:108 in their open-source repo). Critical design decision (design doc section 4.3): the OpenAI / Anthropic providers use a per-request fetch wrapper rather than the SDK defaultHeaders option, because content-generator SDK clients are constructed once and NOT recreated on /clear-triggered session resets (Config.resetSession updates this.sessionId but the contentGenerator keeps using the stale header value). Reading config.getSessionId() from inside the wrapper at request time gives the live value. Gemini provider uses static httpOptions.headers — @google/genai HttpOptions interface does not expose a fetch hook (only headers, baseUrl, apiVersion, timeout, extraParams). This is a known limitation: after session reset, Gemini X-Qwen-Code-Session-Id stays stale until the contentGenerator is recreated. Documented in telemetry.md and the design doc section 8.6; spans/logs continue to carry the live session id for trace/log correlation. Lazy-invalidate fix is a follow-up sub-issue. Header is omitted when telemetry is disabled OR when getSessionId returns an empty string (some HTTP middleware rejects empty header values). Integration sites: - packages/core/src/core/openaiContentGenerator/provider/default.ts (base class — automatically covered by deepseek/minimax/mistral/ modelscope/openrouter subclasses; openrouter calls super.buildHeaders) - packages/core/src/core/openaiContentGenerator/provider/dashscope.ts (overrides buildClient — must be touched separately; QwenContentGenerator inherits via this provider) - packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.ts - packages/core/src/core/geminiContentGenerator/index.ts (factory function, not the GeminiContentGenerator class — no signature change) End-to-end verification (local HTTP server in tmux): PASS: traceparent + X-Qwen-Code-Session-Id on every LLM request PASS: session id refreshes after simulated /clear (staleness regression guarded by llm-correlation-fetch.test.ts) PASS: OTLP upload traffic not traced (no feedback loop — PR A ignoreRequestHook working) Robot generated with Qwen Code https://github.com/QwenLM/qwen-code
📋 Review SummaryThis PR adds a product-namespaced 🔍 General Feedback
🎯 Specific Feedback🟡 High
🟢 Medium
🔵 Low
✅ Highlights
|
There was a problem hiding this comment.
Pull request overview
This PR extends the outbound-LLM telemetry propagation work (stacked on #4390) by adding a product-namespaced session correlation header (X-Qwen-Code-Session-Id) to outbound LLM requests when telemetry is enabled, enabling correlation on the server side without needing to parse trace context.
Changes:
- Introduces
wrapFetchWithCorrelation()/staticCorrelationHeaders()helpers to injectX-Qwen-Code-Session-Id(per-request when possible, static for Gemini). - Hooks the per-request fetch wrapper into OpenAI-compatible and Anthropic SDK client construction; injects static headers for Gemini due to SDK limitations.
- Adds unit tests and updates provider tests, plus developer documentation describing behavior and Gemini’s known limitation.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/telemetry/llm-correlation-fetch.ts | Adds correlation header helpers (per-request fetch wrapper + static headers). |
| packages/core/src/telemetry/llm-correlation-fetch.test.ts | Adds unit tests covering wrapper/header behavior and staleness regression. |
| packages/core/src/core/openaiContentGenerator/provider/default.ts | Installs correlation fetch wrapper on OpenAI SDK client. |
| packages/core/src/core/openaiContentGenerator/provider/default.test.ts | Asserts OpenAI client receives the wrapped fetch. |
| packages/core/src/core/openaiContentGenerator/provider/dashscope.ts | Installs correlation fetch wrapper for DashScope OpenAI-compatible client. |
| packages/core/src/core/openaiContentGenerator/provider/dashscope.test.ts | Asserts DashScope OpenAI client receives the wrapped fetch. |
| packages/core/src/core/geminiContentGenerator/index.ts | Merges static correlation headers into Gemini httpOptions.headers. |
| packages/core/src/core/geminiContentGenerator/index.test.ts | Tests header presence/absence based on telemetry enabled flag. |
| packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.ts | Installs correlation fetch wrapper on Anthropic SDK client. |
| docs/developers/development/telemetry.md | Documents the session correlation header and Gemini limitation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review feedback on #4390: 1. CI was failing on npm ci because the lockfile was generated with npm 11 locally (it sprinkles `peer: true` annotations npm 10 reads differently and rejects). Regenerated with npm 10 (matching CI's Node 22.x default), so the diff vs main is now 18 lines (the actual instrumentation-undici entry) instead of 105 lines of npm-version drift noise. 2. (Copilot inline at sdk.ts:330) `otlpUrlPrefixes` was derived from raw Config strings, so a settings.json `"otlpEndpoint": "\"http://...\""` (quoted) or trailing `#fragment` would silently miss the prefix match and reintroduce the feedback loop the hook exists to prevent. Replaced the regex-based suffix trim with a WHATWG URL parser: - strips ?query, #fragment, trailing slash - trims symmetric ASCII quotes a user may have placed in settings.json - falls back to safe suffix trimming if URL parsing fails (misconfigured endpoint still gets SOME protection) 3. (CodeQL inline) Replaced the `/\?.*$/` regex in ignoreRequestHook with `indexOf('?')`/`indexOf('#')` slicing for ReDoS hygiene. The regex was linear in practice but flagged as polynomial — using indexOf removes the ambiguity and is arguably simpler. Added 3 tests in sdk.test.ts covering the new normalizations (#fragment on incoming path, quoted endpoint, #fragment on configured endpoint). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
…ry safety Adopts 7 review findings from wenshao on #4390 (+ duplicates from now-closed #4393). Critical bugs first, polish second. CRITICAL: 1. tsc TS2322 — wrapper return type incompatible with Anthropic SDK Fetch. `typeof fetch` (Node WHATWG, 2 overloads) is not structurally assignable to Anthropic's narrower `Fetch = (input: RequestInfo, init?) => ...`, even though they're call-compatible at runtime. Make wrapper generic `<TFetch extends FetchLikeLoose>` so callers preserve their exact fetch signature; cast the Anthropic call site through `unknown` with a comment explaining why. 2. tsc TS2352 / TS2493 — `baseFetch.mock.calls[0]![1] as RequestInit` was out-of-bounds when wrapped was called with no init arg. Replaced with a `makeFetchMock()` helper returning typed accessors. 3. normalizeOtlpPrefix catch fallback was DANGEROUS — a config of `"http"` produced prefix `"http"` which `startsWith`-matched every outbound HTTP request → silently disabled ALL instrumentation (no client spans, no correlation header — defeats the entire feature). Fixed: catch returns undefined + diag.warn. Misconfigured endpoint loses its feedback-loop guard (acceptable) instead of disabling all guards (catastrophic). 4. `url.startsWith(prefix)` matching was NOT boundary-safe — port collision (`:4318` matches `:43180`), hostname suffix collision (`otlp.example.com` matches `otlp.example.com.evil.net`), path-segment collision (`/v1` matches `/v1foo/x`). Replaced with origin-equality + path-prefix + boundary-char check (next char must be `/`, `?`, `#`, or end-of-string). 5. HttpInstrumentation also lacked the OTLP feedback-loop guard. The OTLP HTTP exporter (`@opentelemetry/exporter-trace-otlp-http`) uses node:http (patched by HttpInstrumentation, NOT undici). Without this, every OTLP upload batch creates a parasitic client span → feedback loop. Added `ignoreOutgoingRequestHook` that reuses the same `matchesOtlpPrefix` / `stripPathSuffix` helpers as the undici instrumentation. SAFETY: 6. Request input + undefined init dropped the Request's own headers (Authorization etc.) because `new Headers(undefined)` → `{...init, headers}` replaced them with just our session header. Fix: when input is a Request and init.headers is unset, seed from input.headers before adding ours. 7. Wrapped fetch had no try/catch — a throwing Config getter or Headers constructor would propagate as TypeError and break the LLM request path. Wrapped header construction in try/catch; on failure, fall through to baseFetch with original init (no header) + diag.warn. Telemetry must never break the model call. COVERAGE: - 3 new sdk.test.ts boundary tests (port/host/path) - 1 new sdk.test.ts normalizeOtlpPrefix catch-branch coverage - 1 new sdk.test.ts HttpInstrumentation OTLP guard test - 1 new sdk.test.ts proxy-mode wrapped-fetch test (default.test.ts) - 1 new anthropic test asserting wrapped fetch installed on Anthropic SDK - 2 new llm-correlation-fetch.test.ts (Request-headers preservation + try/catch fall-through) All 668 tests pass (1 pre-existing Anthropic User-Agent failure on main is unrelated). tsc clean. Declined: #10 DRY-refactor of baseFetch extraction across 3 sites — the duplication was pre-existing (default/dashscope buildClient was already near-identical), refactoring is a separate cleanup PR not gated by this feature. Will reply on the thread. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
…ry safety Adopts 7 review findings from wenshao on #4390 (+ duplicates from now-closed #4393). Critical bugs first, polish second. CRITICAL: 1. tsc TS2322 — wrapper return type incompatible with Anthropic SDK Fetch. `typeof fetch` (Node WHATWG, 2 overloads) is not structurally assignable to Anthropic's narrower `Fetch = (input: RequestInfo, init?) => ...`, even though they're call-compatible at runtime. Make wrapper generic `<TFetch extends FetchLikeLoose>` so callers preserve their exact fetch signature; cast the Anthropic call site through `unknown` with a comment explaining why. 2. tsc TS2352 / TS2493 — `baseFetch.mock.calls[0]![1] as RequestInit` was out-of-bounds when wrapped was called with no init arg. Replaced with a `makeFetchMock()` helper returning typed accessors. 3. normalizeOtlpPrefix catch fallback was DANGEROUS — a config of `"http"` produced prefix `"http"` which `startsWith`-matched every outbound HTTP request → silently disabled ALL instrumentation (no client spans, no correlation header — defeats the entire feature). Fixed: catch returns undefined + diag.warn. Misconfigured endpoint loses its feedback-loop guard (acceptable) instead of disabling all guards (catastrophic). 4. `url.startsWith(prefix)` matching was NOT boundary-safe — port collision (`:4318` matches `:43180`), hostname suffix collision (`otlp.example.com` matches `otlp.example.com.evil.net`), path-segment collision (`/v1` matches `/v1foo/x`). Replaced with origin-equality + path-prefix + boundary-char check (next char must be `/`, `?`, `#`, or end-of-string). 5. HttpInstrumentation also lacked the OTLP feedback-loop guard. The OTLP HTTP exporter (`@opentelemetry/exporter-trace-otlp-http`) uses node:http (patched by HttpInstrumentation, NOT undici). Without this, every OTLP upload batch creates a parasitic client span → feedback loop. Added `ignoreOutgoingRequestHook` that reuses the same `matchesOtlpPrefix` / `stripPathSuffix` helpers as the undici instrumentation. SAFETY: 6. Request input + undefined init dropped the Request's own headers (Authorization etc.) because `new Headers(undefined)` → `{...init, headers}` replaced them with just our session header. Fix: when input is a Request and init.headers is unset, seed from input.headers before adding ours. 7. Wrapped fetch had no try/catch — a throwing Config getter or Headers constructor would propagate as TypeError and break the LLM request path. Wrapped header construction in try/catch; on failure, fall through to baseFetch with original init (no header) + diag.warn. Telemetry must never break the model call. COVERAGE: - 3 new sdk.test.ts boundary tests (port/host/path) - 1 new sdk.test.ts normalizeOtlpPrefix catch-branch coverage - 1 new sdk.test.ts HttpInstrumentation OTLP guard test - 1 new sdk.test.ts proxy-mode wrapped-fetch test (default.test.ts) - 1 new anthropic test asserting wrapped fetch installed on Anthropic SDK - 2 new llm-correlation-fetch.test.ts (Request-headers preservation + try/catch fall-through) All 668 tests pass (1 pre-existing Anthropic User-Agent failure on main is unrelated). tsc clean. Declined: #10 DRY-refactor of baseFetch extraction across 3 sites — the duplication was pre-existing (default/dashscope buildClient was already near-identical), refactoring is a separate cleanup PR not gated by this feature. Will reply on the thread. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
…gation (#4384) (#4390) * feat(telemetry): propagate W3C traceparent on outbound LLM requests Part 1 of #4384 (sub-issue of #3731 P3 deeper observability). Today qwen-code's only OTel instrumentation is `HttpInstrumentation`, which only patches Node's `http`/`https` modules. The `openai` and `@google/genai` SDKs use `globalThis.fetch` (undici), so outbound LLM requests carry no `traceparent` header and trace context dies at the qwen-code process boundary. Adds `@opentelemetry/instrumentation-undici@0.14.0` (peer-compatible with the installed `@opentelemetry/instrumentation@0.203.0`) and wires it into `initializeTelemetry()` next to the existing `HttpInstrumentation`. Default propagator (W3C tracecontext + baggage) remains unchanged — no explicit `textMapPropagator` needed. `ignoreRequestHook` skips OTLP exporter endpoints to avoid the classic feedback loop (OTel SDK uses fetch to upload OTLP data; without the hook each upload would create a span that gets uploaded, infinitely). Configured `otlpEndpoint` / per-signal endpoints are stripped of trailing slash and query string for robust prefix matching against undici's `request.origin + request.path`. Outbound LLM calls now also produce a client-side HTTP span (separating network TTFB / transfer time from the existing `api.generateContent` total-duration span). Design doc: docs/design/telemetry-outbound-propagation-design.md (Part A — traceparent; Part B — session id header — lands in a follow-up PR per the design's split rationale.) 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(telemetry): harden OTLP feedback-loop guard + slim lockfile diff Review feedback on #4390: 1. CI was failing on npm ci because the lockfile was generated with npm 11 locally (it sprinkles `peer: true` annotations npm 10 reads differently and rejects). Regenerated with npm 10 (matching CI's Node 22.x default), so the diff vs main is now 18 lines (the actual instrumentation-undici entry) instead of 105 lines of npm-version drift noise. 2. (Copilot inline at sdk.ts:330) `otlpUrlPrefixes` was derived from raw Config strings, so a settings.json `"otlpEndpoint": "\"http://...\""` (quoted) or trailing `#fragment` would silently miss the prefix match and reintroduce the feedback loop the hook exists to prevent. Replaced the regex-based suffix trim with a WHATWG URL parser: - strips ?query, #fragment, trailing slash - trims symmetric ASCII quotes a user may have placed in settings.json - falls back to safe suffix trimming if URL parsing fails (misconfigured endpoint still gets SOME protection) 3. (CodeQL inline) Replaced the `/\?.*$/` regex in ignoreRequestHook with `indexOf('?')`/`indexOf('#')` slicing for ReDoS hygiene. The regex was linear in practice but flagged as polynomial — using indexOf removes the ambiguity and is arguably simpler. Added 3 tests in sdk.test.ts covering the new normalizations (#fragment on incoming path, quoted endpoint, #fragment on configured endpoint). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * feat(telemetry): propagate X-Qwen-Code-Session-Id on outbound LLM requests Part 2 of #4384. Stacks on top of PR #4390 (traceparent via undici). Adds a product-namespaced HTTP header X-Qwen-Code-Session-Id to every outbound LLM request when telemetry is enabled, so server-side ingestion can correlate observed requests with qwen-code session metric/log records. Pattern matched from claude-code (X-Claude-Code-Session-Id, verified at src/services/api/client.ts:108 in their open-source repo). Critical design decision (design doc section 4.3): the OpenAI / Anthropic providers use a per-request fetch wrapper rather than the SDK defaultHeaders option, because content-generator SDK clients are constructed once and NOT recreated on /clear-triggered session resets (Config.resetSession updates this.sessionId but the contentGenerator keeps using the stale header value). Reading config.getSessionId() from inside the wrapper at request time gives the live value. Gemini provider uses static httpOptions.headers — @google/genai HttpOptions interface does not expose a fetch hook (only headers, baseUrl, apiVersion, timeout, extraParams). This is a known limitation: after session reset, Gemini X-Qwen-Code-Session-Id stays stale until the contentGenerator is recreated. Documented in telemetry.md and the design doc section 8.6; spans/logs continue to carry the live session id for trace/log correlation. Lazy-invalidate fix is a follow-up sub-issue. Header is omitted when telemetry is disabled OR when getSessionId returns an empty string (some HTTP middleware rejects empty header values). Integration sites: - packages/core/src/core/openaiContentGenerator/provider/default.ts (base class — automatically covered by deepseek/minimax/mistral/ modelscope/openrouter subclasses; openrouter calls super.buildHeaders) - packages/core/src/core/openaiContentGenerator/provider/dashscope.ts (overrides buildClient — must be touched separately; QwenContentGenerator inherits via this provider) - packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.ts - packages/core/src/core/geminiContentGenerator/index.ts (factory function, not the GeminiContentGenerator class — no signature change) End-to-end verification (local HTTP server in tmux): PASS: traceparent + X-Qwen-Code-Session-Id on every LLM request PASS: session id refreshes after simulated /clear (staleness regression guarded by llm-correlation-fetch.test.ts) PASS: OTLP upload traffic not traced (no feedback loop — PR A ignoreRequestHook working) Robot generated with Qwen Code https://github.com/QwenLM/qwen-code * fix(telemetry): R2 review fixes — critical correctness + tsc + boundary safety Adopts 7 review findings from wenshao on #4390 (+ duplicates from now-closed #4393). Critical bugs first, polish second. CRITICAL: 1. tsc TS2322 — wrapper return type incompatible with Anthropic SDK Fetch. `typeof fetch` (Node WHATWG, 2 overloads) is not structurally assignable to Anthropic's narrower `Fetch = (input: RequestInfo, init?) => ...`, even though they're call-compatible at runtime. Make wrapper generic `<TFetch extends FetchLikeLoose>` so callers preserve their exact fetch signature; cast the Anthropic call site through `unknown` with a comment explaining why. 2. tsc TS2352 / TS2493 — `baseFetch.mock.calls[0]![1] as RequestInit` was out-of-bounds when wrapped was called with no init arg. Replaced with a `makeFetchMock()` helper returning typed accessors. 3. normalizeOtlpPrefix catch fallback was DANGEROUS — a config of `"http"` produced prefix `"http"` which `startsWith`-matched every outbound HTTP request → silently disabled ALL instrumentation (no client spans, no correlation header — defeats the entire feature). Fixed: catch returns undefined + diag.warn. Misconfigured endpoint loses its feedback-loop guard (acceptable) instead of disabling all guards (catastrophic). 4. `url.startsWith(prefix)` matching was NOT boundary-safe — port collision (`:4318` matches `:43180`), hostname suffix collision (`otlp.example.com` matches `otlp.example.com.evil.net`), path-segment collision (`/v1` matches `/v1foo/x`). Replaced with origin-equality + path-prefix + boundary-char check (next char must be `/`, `?`, `#`, or end-of-string). 5. HttpInstrumentation also lacked the OTLP feedback-loop guard. The OTLP HTTP exporter (`@opentelemetry/exporter-trace-otlp-http`) uses node:http (patched by HttpInstrumentation, NOT undici). Without this, every OTLP upload batch creates a parasitic client span → feedback loop. Added `ignoreOutgoingRequestHook` that reuses the same `matchesOtlpPrefix` / `stripPathSuffix` helpers as the undici instrumentation. SAFETY: 6. Request input + undefined init dropped the Request's own headers (Authorization etc.) because `new Headers(undefined)` → `{...init, headers}` replaced them with just our session header. Fix: when input is a Request and init.headers is unset, seed from input.headers before adding ours. 7. Wrapped fetch had no try/catch — a throwing Config getter or Headers constructor would propagate as TypeError and break the LLM request path. Wrapped header construction in try/catch; on failure, fall through to baseFetch with original init (no header) + diag.warn. Telemetry must never break the model call. COVERAGE: - 3 new sdk.test.ts boundary tests (port/host/path) - 1 new sdk.test.ts normalizeOtlpPrefix catch-branch coverage - 1 new sdk.test.ts HttpInstrumentation OTLP guard test - 1 new sdk.test.ts proxy-mode wrapped-fetch test (default.test.ts) - 1 new anthropic test asserting wrapped fetch installed on Anthropic SDK - 2 new llm-correlation-fetch.test.ts (Request-headers preservation + try/catch fall-through) All 668 tests pass (1 pre-existing Anthropic User-Agent failure on main is unrelated). tsc clean. Declined: #10 DRY-refactor of baseFetch extraction across 3 sites — the duplication was pre-existing (default/dashscope buildClient was already near-identical), refactoring is a separate cleanup PR not gated by this feature. Will reply on the thread. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * chore(deps): allow patch updates for @opentelemetry/instrumentation-undici Switch from exact pin `0.14.0` to `^0.14.0` for consistency with the rest of the `@opentelemetry/*` deps in this block (all carated). For 0.x semver, npm treats `^0.14.0` as `>=0.14.0 <0.15.0`, so patch updates within the 0.14.x line — which are tied to the same `@opentelemetry/instrumentation@0.203.x` peer — flow in via `npm update` without requiring a manual package.json edit. A bump across the 0.x minor (e.g. 0.15.x) would shift the instrumentation peer compatibility and still requires explicit attention, which the caret correctly blocks. Per review feedback on #4390 (wenshao). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * test(telemetry): stub getTelemetryEnabled + getSessionId in Gemini factory tests The X-Qwen-Code-Session-Id commit added a `staticCorrelationHeaders(gcConfig)` call inside the Gemini content generator factory. That helper reads `gcConfig.getTelemetryEnabled()` and `gcConfig.getSessionId()` per request. Both pre-existing Gemini tests in `contentGenerator.test.ts` build a minimal partial Config stub via `as unknown as Config` and only stub the methods the factory used to need. The new call path now hits the unstubbed methods at runtime, surfacing as `TypeError: config.getTelemetryEnabled is not a function` on all three CI platforms. Add the two missing stubs to both test cases. The Gemini factory continues to ignore the values when telemetry is off — these stubs only have to exist, not return anything in particular. Local check ran the full test suite for the four directories `/loop` covers plus `src/core/contentGenerator.test.ts` itself; all green. Also re-ran the other test files that build partial Config mocks via the same idiom (`client.test.ts`, `config.test.ts`, `nextSpeakerChecker.test.ts`, `content-generator-config.test.ts`) — none exercise the new code path. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(telemetry): R3 review fixes — port + protocol + quote + safety Four issues found by wenshao reviewing the R2 boundary-safety pass on PR #4390. All four close gaps where the OTLP feedback-loop guard or the correlation-header path could fail silently. 1. **Port normalization mismatch** (sdk.ts ignoreOutgoingRequestHook): `normalizeOtlpPrefix` builds prefixes via `URL.origin`, which strips default ports (`:80` for http, `:443` for https). The hook reconstructed request origin manually as `${proto}://${host}${portPart}`, keeping the port. Result: prefix `http://collector` (no explicit port) didn't match a request to `http://collector:80/v1/traces` because their `.origin` differed → guard bypassed → feedback loop. Now the reconstructed origin is also routed through `URL` so both sides apply the same default-port stripping. 2. **HTTPS proto silent fallback** (sdk.ts ignoreOutgoingRequestHook): The `(req.protocol && ...) || 'http'` fallback would silently mis-bucket HTTPS requests as HTTP when `req.protocol` was unset, so HTTPS OTLP endpoints couldn't match their prefix. Changed to fail open: when proto can't be determined, return false (request gets instrumented). Worst case is a parasitic client span — observable, recoverable — versus the previous unbounded silent feedback loop. Picked fail-open over the bot's port-based heuristic because non-standard HTTPS ports break the heuristic but not fail-open. 3. **Quote-stripping divergence** (sdk.ts normalizeOtlpPrefix): `parseOtlpEndpoint` (line 109) uses `/^["']|["']$/g` which strips asymmetric leading/trailing quotes; `normalizeOtlpPrefix` previously only stripped symmetric pairs. A settings.json typo like `"value'` would let the exporter connect (parseOtlpEndpoint trims) but leave the guard returning `undefined` (normalizeOtlpPrefix rejected) → parasitic loop. Aligned `normalizeOtlpPrefix` to the same lenient regex. 4. **`staticCorrelationHeaders` missing try/catch** (llm-correlation-fetch.ts): `wrapFetchWithCorrelation` already catches all internal exceptions and falls through to baseFetch — same "telemetry must never break LLM path" contract was missing on the static-headers helper. A throw here would propagate up to the Gemini content-generator factory and crash content-generator init for the whole session. Wrapped the body in try/catch with `diag.warn` fall-through to `{}`. Tests: added 4 regression tests covering each scenario: - default-port HTTP request matched against portless prefix (1) - hook returns false when req.protocol missing on https endpoint (2) - asymmetric-quoted endpoint normalizes for guard parity (3) - staticCorrelationHeaders returns {} when config getter throws (4) 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * docs(telemetry): fix misleading "BOTH" wording in wrapFetchWithCorrelation The comment described the header-seeding logic as merging "BOTH the init.headers AND the Request's own headers", but the two branches are mutually exclusive — `new Headers(init?.headers)` runs unconditionally (empty Headers when init.headers is undefined), and the Request-headers copy only runs when init.headers is undefined. So in practice it's either-or, not BOTH. Reworded to match the actual logic per #4390 review feedback (wenshao). Behavior unchanged. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(telemetry): strip port from req.host fallback + document undici scope Two issues found by wenshao reviewing the R3 boundary-safety fixes on PR #4390. 1. **`req.host` may already include `:port`** (sdk.ts ignoreOutgoingRequestHook): When `req.hostname` is absent and `req.host` is the fallback, the value may already be `"collector:4318"`. Naively appending `:${req.port}` produced `"http://collector:4318:4318"` → `new URL()` rejects → catch returns false → silent guard bypass for that request. Currently unreachable because `@opentelemetry/otlp-exporter-base` always sets `hostname` from WHATWG URL parsing, but the fallback exists in the code and must be correct — a future OTLP transport that emits `host` without `hostname` would silently trigger the feedback loop. Strip the port when falling back; bracketed IPv6 literals like `"[::1]:443"` keep their bracketed host intact. 2. **Undici scope honesty** (telemetry.md): Previous docs framed the propagation as "outbound LLM requests", but `UndiciInstrumentation` actually patches `globalThis.fetch` for the whole process — `WebFetch`, MCP clients, IDE extension calls all get spans + `traceparent` injection too. Added a "Scope: all fetch() calls, not just LLM" subsection covering: (a) trace ID leakage to third-party URLs (the user-supplied destinations of `WebFetch` see our trace ID; not secret per W3C but worth knowing); (b) non-LLM span volume inflating OTLP batches with a workaround tip. Per-destination scoping toggle deferred as a follow-up — out of scope for this PR. Added regression test for the host:port-fallback path. Test exercises the previously broken combination (hostname absent, host carries port) through the existing test harness. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * feat(telemetry): scope X-Qwen-Code-Session-Id to first-party hosts by default Address LaZzyMan's REQUEST_CHANGES review of PR #4390. The original design injected `X-Qwen-Code-Session-Id` on every outbound LLM request gated only by `telemetry.enabled`. Review caught that this broadcasts a stable cross-request client identifier to every configured third-party provider (OpenAI, Anthropic, OpenRouter, MiniMax, ModelScope, Mistral, vanilla Gemini, ...), which the claude-code precedent does NOT justify — claude-code is a first-party Anthropic→Anthropic flow; qwen-code is an open-source CLI connecting to many providers. Fix: add a host allowlist with a deliberately narrow default. The header is now only attached to destinations whose hostname matches: dashscope.aliyuncs.com dashscope-intl.aliyuncs.com *.dashscope.aliyuncs.com *.dashscope-intl.aliyuncs.com *.alibaba-inc.com *.aliyun-inc.com This is exactly the set where the LLM provider, the upstream telemetry backend (ARMS Tracing), and qwen-code itself are the same legal entity — mirroring the first-party claude-code pattern and preserving the real product value (server-side trace stitching against DashScope) without exposing the session id to third parties. Operators with broader correlation requirements override via: "telemetry": { "sessionIdHeaderHosts": ["*"] // restore broadcast "sessionIdHeaderHosts": [] // fully disable "sessionIdHeaderHosts": ["api.example.com", "*.foo"] // custom allowlist } Implementation: - NEW `telemetry/trusted-llm-hosts.ts`: `DEFAULT_SESSION_ID_HEADER_HOSTS` + `matchesTrustedHost(hostname, patterns)` + `extractRequestHost(input)`. Pattern syntax is intentionally tiny (bare hostname OR `*.suffix`, dot-anchored to reject `evil-alibaba-inc.com` style attacks). Unit-tested in dedicated test file including TLD/sub-domain attack vectors. - `wrapFetchWithCorrelation` (openai + anthropic providers): resolves the allowlist at wrap time (Config snapshot), inspects each request's destination URL inside `correlationFetch`, falls through to baseFetch for non-trusted destinations. Wildcard escape hatch via `["*"]`. - `staticCorrelationHeaders` (Gemini factory): now takes an optional `destinationUrl` and applies the same host gate. The Gemini SDK default endpoint `generativelanguage.googleapis.com` is NOT on the default allowlist, so vanilla Gemini calls receive no header — matching the "first-party only" scope. Operators who put the Gemini SDK on a DashScope-compatible endpoint via `baseUrl` get the header naturally. - `Config.getTelemetrySessionIdHeaderHosts()` getter + `TelemetrySettings.sessionIdHeaderHosts` interface field + JSON schema entry in `settingsSchema.ts`. Wired through `resolveTelemetrySettings`. - Defensive optional-chaining + try/catch on the Config getter call at wrap time so partial test mocks (or pre-getter Config implementations) fall back to the default allowlist rather than crashing buildClient. Tests: 12 new cases covering host match/skip on default allowlist, sub-domain handling, TLD-suffix attack rejection, `["*"]` broadcast override, `[]` full-disable, custom operator allowlist, unparseable destination (fail closed), and the three Gemini factory paths (googleapis.com default → omit; DashScope `baseUrl` → inject; custom allowlist → inject). Docs updated in `docs/developers/development/telemetry.md` Session correlation header section, including override examples and the new Gemini host-gate semantics. Closes the LaZzyMan REQUEST_CHANGES blocker. The cross-vendor fingerprint-broadcast failure mode is now opt-in rather than default, restoring the first-party-only semantics that make the claude-code precedent applicable. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(telemetry): R5 review fixups — Vertex destination + ["*"] trim + docs Self-review pass on commit 1c8528a (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) * chore: regenerate settings.schema.json for sessionIdHeaderHosts Lint check `Check settings schema is up-to-date` failed because the checked-in `packages/vscode-ide-companion/schemas/settings.schema.json` wasn't regenerated after adding `telemetry.sessionIdHeaderHosts` to `settingsSchema.ts` in commit 1c8528a. Regenerated via `npm run generate:settings-schema`. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * docs(design): update telemetry-outbound-propagation design for R3 host-allowlist scoping Adds a "修订历史" header table at the top and a new §11 "R3 修订 — Host-Allowlist Scoping for X-Qwen-Code-Session-Id" capturing what changed after LaZzyMan's REQUEST_CHANGES review, why, and how. Inline pointers added at §3.1, §3.2, §4.3, §4.4, §9 (claude-code comparison table) to point readers at §11 — original prose preserved as a record of the decision path rather than rewritten in place. Concretely §11 covers: - The three-step LazzyMan critique and why R1's "broadcast to all providers" was structurally wrong for an open-source multi-provider CLI - The default allowlist (`DEFAULT_SESSION_ID_HEADER_HOSTS`) and its semantic alignment with the DashScope provider detector - Pattern grammar (bare hostname / `*.suffix` dot-anchored), the TLD-suffix attack vectors it rejects, why no regex / port-aware globbing - `wrapFetchWithCorrelation` host gate, wrap-time vs request-time semantics, `[" * "]` whitespace tolerance - `staticCorrelationHeaders` `destinationUrl` parameter, Gemini factory's fail-closed treatment of unset `baseUrl` (avoids the Vertex vs `generativelanguage.googleapis.com` ambiguity) - All R3 file changes mapped to the original §5 file-change list - Mapping of LazzyMan's three concerns to R3's responses - §10 future-work additions: `traceparent` per-destination toggle, `X-Qwen-Code-Request-Id`, IPv6 allowlist syntax No code changes; documentation only. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * fix(telemetry): defensive allowlist normalization + positive proxy test Three issues found by wenshao reviewing the R3 host-allowlist scoping. 1. **[Critical] `broadcastAll` outside safety try/catch** (llm-correlation-fetch.ts wrapFetchWithCorrelation) The try/catch only fires when `getTelemetrySessionIdHeaderHosts()` throws. If it returns a malformed value — a bare string (settings.json typo `"sessionIdHeaderHosts": "host"` instead of `["host"]`), an array containing `null`/`undefined`/number entries, or whitespace-padded entries — `.some((p) => p.trim() === '*')` throws TypeError at buildClient time, bricking the LLM session before the first prompt. `staticCorrelationHeaders` already handled this via its end-to-end try/catch but the sister helper diverged. Settings loader does no runtime schema validation so this is reachable via a single typo. Fix: normalize the allowlist at wrap time: 1. catch a throwing getter (existing) 2. reject non-array → default allowlist (NEW — bare string typo) 3. filter out non-string elements (NEW — [null, ...] typo) 4. trim every surviving entry uniformly (NEW — see #2 below) Then `trustedHosts.includes('*')` instead of `.some((p) => p.trim() === '*')`, since patterns are already pre-trimmed. 2. **Trim asymmetry between `*` detection and host-pattern match** (llm-correlation-fetch.ts) `[" * "]` was tolerated (trimmed before `===` compare) but `[" dashscope.aliyuncs.com "]` silently never matched. The normalization above fixes this by trimming uniformly upstream. 3. **Proxy fetch test: only negative assertions** (openaiContentGenerator/provider/default.test.ts) The test asserted `callArg.fetch !== proxyFetch` and `!== globalThis.fetch` but both passed for ANY wrapper, including a buggy one that accidentally wraps globalThis.fetch instead of proxyFetch. Added a positive assertion: call the wrapped fetch and verify proxyFetch was the delegation target. Tests: 4 new cases — whitespace-padded host pattern, bare-string malformed config (both wrapper and static), null/number-containing array malformed config (both wrapper and static), positive proxy fetch delegation. All pass; pre-existing Anthropic User-Agent failure unrelated. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * refactor(telemetry): split outbound correlation out of telemetry scope (R4) Address LaZzyMan round-8 follow-up review on PR #4390: even though R3's host allowlist made the default behavior safe, the meta-architectural concern remains: telemetry's namespace and consent flow shouldn't quietly extend to wire-level behavior aimed at third-party LLM provider request streams. The recipient sets differ; the consent decisions differ; they deserve separate namespaces, separate threat models, separate PRs. This commit (called R4 in the design doc) collapses the PR scope so it lands ONLY telemetry observability work: REMOVED from this PR: - packages/core/src/telemetry/llm-correlation-fetch.ts(.test.ts) - packages/core/src/telemetry/trusted-llm-hosts.ts(.test.ts) - telemetry.sessionIdHeaderHosts setting + Config getter + resolveTelemetrySettings wiring + settingsSchema entry - wrapFetchWithCorrelation usage from four provider construction points (default.ts, dashscope.ts, anthropicContentGenerator.ts, geminiContentGenerator/index.ts) - All session-id provider tests across the four providers + the contentGenerator.test.ts mock stub - "Session correlation header" section in telemetry.md ADDED: - OutboundCorrelationSettings interface in packages/core/src/config/config.ts, standalone top-level namespace separate from TelemetrySettings — SECURITY-RELEVANT label, all defaults off - Config.getOutboundCorrelationPropagateTraceContext() getter - outboundCorrelation top-level entry in settingsSchema.ts with propagateTraceContext: { default: false } and explicit SECURITY-RELEVANT framing in the description - CLI config-load pipeline passes settings.outboundCorrelation into ConfigParameters - NOOP_PROPAGATOR (TextMapPropagator no-op) in sdk.ts, conditionally installed on NodeSDK when propagateTraceContext is false (default). When true, omits textMapPropagator from NodeSDK options so the SDK keeps its default W3C composite propagator - 2 new sdk.test.ts cases covering the propagator gate behavior UNCHANGED: - UndiciInstrumentation registration + OTLP feedback-loop guard + HttpInstrumentation OTLP guard from R2/R3 stay intact — they are pure telemetry (client HTTP spans into the operator's own OTLP collector), no wire-level data egress - Documentation rewrites telemetry.md to split "client-side HTTP span on outbound fetch" (telemetry) from a new "Outbound correlation (SECURITY-RELEVANT)" top-level section - design doc gets R4 revision row + new §12 "R4 Scope Conflation Split" capturing the rationale and follow-up PR outline The session-id apparatus (R3 code) lives in git history at commits 1c8528a / cb162e7 / 7a1b4f8 / 40e1efc / 106598c; the follow-up PR can cherry-pick or restore those files under the new outboundCorrelation.* namespace as LazzyMan suggested. Vscode-ide-companion settings.schema.json regenerated. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * docs(telemetry): disclose telemetry.enabled dependency on propagateTraceContext Self-review pass on R4 commit 9bdd3bd flagged one footgun: both `docs/developers/development/telemetry.md` and the settingsSchema.ts description for `outboundCorrelation.propagateTraceContext` describe the toggle's behavior without noting that the flag is a silent no-op when `telemetry.enabled` is false. An operator who sets only `outboundCorrelation.propagateTraceContext: true` and forgets the telemetry switch gets zero behavior change — no error, no warning, no traceparent. Fix: add the dependency disclosure to both surfaces, plus a JSON example showing both flags wired together for the ARMS+DashScope cross-process trace continuation use case. Also fix a minor comment accuracy nit at `sdk.test.ts:683`: said the SDK installs `W3CTraceContextPropagator` instance when opt-in is true, but the actual default is `CompositePropagator(W3CTraceContextPropagator + W3CBaggagePropagator)` per `@opentelemetry/sdk-node` source. Vscode-ide-companion settings.schema.json regenerated to reflect the expanded description string. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * test(config): cover getOutboundCorrelationPropagateTraceContext defaults R4 (commit 9bdd3bd) added the getter but the test file didn't grow a corresponding describe block — sibling telemetry getters all have unit tests but this new one was missed. Add 4 cases covering the security-relevant default-to-false invariant and explicit-set behavior: - omitted outboundCorrelation → false - empty outboundCorrelation: {} → false (the `?? false` collapse on the getter, complementing the same on the constructor) - explicit true → true - explicit false → false PR #4390 review (wenshao). 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * docs(design): reflect post-R4 polish commits in §12 R4 (commit 9bdd3bd) was followed by two polish commits that the design doc §12 didn't track: - 0be0df2 (docs): telemetry.enabled dependency disclosure on propagateTraceContext — added to telemetry.md + settingsSchema description because a self-review pass identified the silent-no-op footgun (operator sets propagateTraceContext: true but forgets telemetry.enabled: true, sees zero behavior change with no error). - c0352fd (test): 4 config.test.ts cases covering the getOutboundCorrelationPropagateTraceContext default-false invariant (omitted / {} / explicit true / explicit false) — wenshao review flagged the test gap. Updates §12.4 with a new "Hidden dependency: telemetry.enabled" sub- section explaining the gating relationship and pointing forward at the follow-up PR (future outboundCorrelation.* settings inherit the same dependency). Updates §12.5 implementation table to add the config.test.ts row and clarify the telemetry.md / vscode-schema rows were touched again in the polish pass. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) * refactor: simplify post-R4 polish per /simplify review /simplify review pass on commits 0be0df2 + c0352fd + 62cf6b4 flagged 4 concerns. Fix all 4: 1. **settingsSchema.ts description**: footgun warning ("Depends on telemetry.enabled: true") was at char 600+ of a 650-char description. VS Code settings UI truncates to ~300 chars inline → the most important warning was hidden in the most-glanced view. Hoist to first sentence ("Requires telemetry.enabled: true."). 2. **config.test.ts**: drop the task-narration comment ("PR #4390 R4: keep wire-level toggle out of telemetry namespace.") that just restated the change context. The remaining 2-line comment explaining WHY default-to-false is security-relevant survives. 3. **config.test.ts**: collapse 4 separate `it()` blocks into a single `it.each([...])` covering the same 4 precondition × expectation combinations. Removes boilerplate (`new Config({...baseParams, ...})` repeated 4×) without losing assertion power; case-3 ("explicit false") was a weak duplicate of case-2 ("empty object") since both hit the same `?? false` branch, but keeping all 4 in the parametric table documents intent more clearly than dropping case-3. 4. **design doc §12.4 + §12.5**: strip specific commit SHAs (`0be0df270`, `c0352fd5b`) — design docs should be evergreen, not doubled-up commit logs (those live in `git log`). Keep the design intent ("two panels both document the dependency" / "test block added") without naming the specific commits. Regenerated vscode-ide-companion/schemas/settings.schema.json to reflect the hoisted description sentence. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code)
Part 2 of #4384. Stacks on top of #4390 (PR A — traceparent via undici instrumentation). Set the base to PR A's branch so the diff shows only this PR's additions.
Summary
Adds a product-namespaced HTTP header
X-Qwen-Code-Session-Idto every outbound LLM request when telemetry is enabled. Pattern matched from claude-code (X-Claude-Code-Session-Id, verified atsrc/services/api/client.ts:108in their open-source repo). Server-side ingestion (e.g. a custom DashScope proxy or OTLP-aware API gateway) can use the header to stitch its observation of an LLM request back to the originating qwen-code session without parsing trace context.Critical design: fetch wrapper, not
defaultHeadersThe OpenAI / Anthropic providers use a per-request fetch wrapper rather than the SDK's
defaultHeadersoption. Why: content-generator SDK clients are constructed ONCE and NOT recreated on/clear-triggered session resets —Config.resetSession()updatesthis.sessionIdbut the contentGenerator keeps using the stale header value baked intodefaultHeaders. Readingconfig.getSessionId()from inside the fetch wrapper at request time gives the live value.This is verified end-to-end by the staleness regression test in
llm-correlation-fetch.test.ts.Known limitation: Gemini
@google/genai'sHttpOptionsinterface does NOT expose afetchhook (onlyheaders/baseUrl/apiVersion/timeout/extraParams). Gemini therefore uses statichttpOptions.headersand shares the staleness bug after/clear. Spans and logs still carry the live session id, so trace/log backends correctly attribute requests to the new session — only the wire-header at the LLM-service boundary is stale. Documented intelemetry.mdand the design doc §8.6. Lazy-invalidate fix is a follow-up sub-issue.Integration sites (4 SDK construction points)
packages/core/src/core/openaiContentGenerator/provider/default.ts(base — 5 sibling providers inherit; openrouter callssuper.buildHeaders)packages/core/src/core/openaiContentGenerator/provider/dashscope.ts(overridesbuildClient; QwenContentGenerator inherits via this)packages/core/src/core/anthropicContentGenerator/anthropicContentGenerator.tspackages/core/src/core/geminiContentGenerator/index.ts(factory, NOT the class — no signature change needed;gcConfigalready in scope)Test plan
llm-correlation-fetch.test.ts— including the staleness regression test (emit request 1, mutate sessionId, emit request 2, assert new id on wire)OpenAI/GoogleGenAIconstructor receives wrapped fetch / merged headers (telemetry on/off cases)/tmp/verify-pr4384.mjs): local HTTP server captures real outbound requestssess-initialset, fire request → captured header hassess-initialsess-after-clear, fire request → captured header has new id (regression-guards the staleness fix in real SDK conditions, not just mocks)traceparent— confirms PR A'signoreRequestHookworkingCaptured output:
```
[0] POST /v1/chat/completions
X-Qwen-Code-Session-Id: sess-initial
traceparent: 00-21278f951b68d6a0c412abfce3427078-81f936f76a72909b-01
[1] POST /v1/chat/completions
X-Qwen-Code-Session-Id: sess-after-clear
traceparent: 00-c71d416430a4612ba407b8a7e83baaf9-170601e26252e2e0-01
```
Audited but not modified
packages/core/src/qwen/qwenContentGenerator.ts— extendsOpenAIContentGeneratorusingDashScopeOpenAICompatibleProvider; auto-inherits the dashscope changepackages/core/src/core/loggingContentGenerator/loggingContentGenerator.ts— wrapper, doesn't construct SDK clientsOut of scope (per design §3.2, future sub-issues)
TRACEPARENTenv var inheritance (BashTool)TRACEPARENTread on startup (--prompt/ Agent SDK modes)X-Qwen-Code-Request-Idper-request UUIDDesign doc:
docs/design/telemetry-outbound-propagation-design.md(landed in PR A).🤖 Generated with Qwen Code