fix(core): honor usage-statistics opt-out for extension lifecycle events - #12789

Critical-only review at d6f637c2 — approving

Base 81c260bb. Eighteen files, +721/-74. This is a privacy-boundary fix, so I verified the two historical Criticals in code at this head and then checked the one completeness question that decides whether the fix actually holds: whether any construction site can still emit a lifecycle event past the opt-out.

Both historical Criticals are fixed at this head

R1-1 [fails-closed] [regression] — the process-global dispatcher leak. Forwarding proxy into the throwaway telemetry Config made its constructor call setGlobalDispatcher, which nothing ever restores, so every extension lifecycle event re-routed the host process's plain fetch — and in qwen serve leaked one workspace's proxy into every other workspace the daemon hosts. The author initially declined this as needing a maintainer decision; it has since been fixed, and I verified the mechanism rather than the intent. ConfigParameters gains installProxyDispatcher?: boolean (config/config.ts:1169) and the constructor's gate is now:

const proxyUrl = this.getProxy();
if (proxyUrl && (params.installProxyDispatcher ?? true)) {

That wraps the whole setGlobalDispatcher + setResolvedProxyUrlForRuntimeFetch block, and the ?? true default means every existing construction site behaves exactly as before — only the telemetry-only Config opts out, at extensionManager.ts:601. getProxy() still returns the value on that Config, so QwenLogger.getProxyAgent() can pin its own agent and the RUM upload still honors the proxy: the global side effect is gone without losing the behavior the forwarding was for. The same method also wraps normalizeProxyUrl(this.proxy) in a try/catch and drops an unsupported value, so a SOCKS proxy cannot make the constructor throw and abort the extension mutation the throwaway exists to record.

R1-2 [certifies-falsely] [new-surface] — ambient env in the workspace-scoped controller. Verified fixed at workspace-extensions-controller.ts:314-340. loadSettings is now a single call carrying skipLoadEnvironment: true, plus skipWorkspaceSettings: !workspaceTrusted and workspaceTrusted when known, so a workspace's .env / settings.env is no longer written into the daemon's shared process.env and the trust value resolved above is the one the load actually uses. The telemetry proxy is settings.proxy alone; the ambient HTTPS_PROXY / https_proxy / HTTP_PROXY / http_proxy fallback is gone, with the multi-workspace reason recorded in the comment. The asymmetry with the CLI command sites is deliberate and correct — those are one-shot processes where ambient env is the right source, while the daemon hosts many workspaces in one process.

The standing CHANGES_REQUESTED is the bot's at the older head 40f203c0; both findings it rests on are closed at this one.

The completeness question

A consent fix only holds if no site can bypass it, so I checked whether the PR's claim — every mutation-capable construction site is wired — survives contact with the tree. A repo-wide search for new ExtensionManager( returns three production sites this diff does not touch: commands/mcp/list.ts, commands/mcp/reconnect.ts and serve/workspace-skills-status.ts, plus a bench script. None of the three contains a single call to installExtension, uninstallExtension, updateExtension, enableExtension or disableExtension.

And every logExtension* call in extensionManager.ts sits inside a mutation method: logExtensionEnable in enableExtension (:772), logExtensionDisable in disableExtension (:823), the install and update events in installExtensionInternal (:2835, :2847, :2938, :2950) and commitPreparedExtensionInternal (:3032, :3044, :3089, :3101), and logExtensionUninstall in uninstallExtensionPolicy (:3324). So a read-only consumer cannot reach a lifecycle event, and the five wired sites — main-session Config, extensions/install.ts, extensions/uninstall.ts, extensions/utils.ts and the daemon controller — are all of the mutation-capable ones. The claim holds.

The shared helper is what keeps them from drifting: resolveUsageStatisticsEnabled is parseBooleanEnvFlag(env['QWEN_USAGE_STATISTICS_ENABLED']) ?? settingsValue ?? true, and cli/src/config/config.ts now calls it instead of carrying its own copy of that chain.

Bonus fix worth naming

qwen-logger.ts gains isExcludedByNoProxy, mirroring undici's EnvHttpProxyAgent#shouldProxy — lowercase no_proxy wins, a bare * disables proxying, entries split on comma or whitespace, a leading . or *. is stripped for subdomain matching, and a port-qualified entry only matches that port. HttpsProxyAgent has no such notion, so without this the newly forwarded proxy would have routed RUM uploads through it even for hosts an operator explicitly excluded. That closes the substance of R1-7 and makes the proxy forwarding safe to have added.

No Critical found. All eight threads are resolved; the six Suggestions are not mine to adjudicate under this scan.

Non-blocking

resolveExtensionTelemetryProxy starts at settings.proxy and omits the leading argv.proxy term the canonical session chain carries, so qwen --proxy … extensions install would upload through the env or settings proxy rather than the flag. Telemetry-only, one-shot command, no consent implication.

CI

Healthy at this head: 22 checks pass with zero failures, including Test (ubuntu-latest, Node 22.x) and Lint & Static, which carry the new regression tests. review-pr and web-shell E2E Smoke are still queued; neither is a code-quality signal and I did not wait on them.

Review at head d6f637c2 -- approving

Base 81c260bb. Eighteen files, +721/-74.

Tier: Standard (privacy boundary).

What I checked

Root cause -- getTelemetryConfig was a module-level function that built a throwaway Config without usageStatisticsEnabled, so the default true was always used regardless of the user's opt-out setting. QwenLogger.getInstance(config) is the sole opt-out gate; it stays closed only when config.getUsageStatisticsEnabled() returns false. The fix: ExtensionManager now accepts usageStatisticsEnabled and proxy, and getTelemetryConfig is promoted to a private method that reads them.

resolveUsageStatisticsEnabled -- Canonical resolver in telemetry/config.ts with the same precedence as the main session: QWEN_USAGE_STATISTICS_ENABLED env, then settings.privacy.usageStatisticsEnabled, then default true. Tests pin the env-override direction and the unrecognized-value (parseBooleanEnvFlag) fallback to false.

resolveExtensionTelemetryProxy -- Returns the raw first match (settings.proxy, then HTTPS_PROXY etc.) without normalizing. Normalization happens in getTelemetryConfig inside a try-catch: a SOCKS proxy or other unsupported value is silently dropped so telemetry never aborts the extension mutation.

installProxyDispatcher: false -- The old getTelemetryConfig never installed the global undici dispatcher. The new path would install it on every lifecycle event in qwen serve, re-routing the daemon's plain fetch through whichever workspace's proxy happened to log last. installProxyDispatcher: false prevents this. The test "forwards the resolved proxy so RUM uploads use it" asserts the global dispatcher is untouched 50ms after the event.

CLI vs daemon asymmetry -- CLI commands (install.ts, uninstall.ts, utils.ts) call resolveExtensionTelemetryProxy(settings.proxy), which falls back to ambient env. The daemon's extension controller uses settings.proxy directly with skipLoadEnvironment: true. One daemon hosts every workspace; the daemon's ambient HTTPS_PROXY is process-global and must not become a workspace-specific telemetry proxy. The test "resolves telemetry proxy and consent from the workspace settings only" pins that no ambient-env leak occurs even when HTTPS_PROXY is set in the daemon's environment.

loadSettings consolidation -- One trust-gated load per createExtensionManager call, shared by locale resolution, trust status check, and telemetry options. Test "loads settings once per manager" pins this at exactly 1 call.

Taxonomy classes checked

Auth/credentials (usage-statistics opt-out gate, proxy routing), contract asymmetry (CLI env fallback vs daemon settings-only -- documented intentional), error handling (SOCKS proxy dropped in getTelemetryConfig), test validity (env teardown in uninstall/utils tests saves and restores all five proxy env keys).

No findings. Ready to merge.

Approved at head d6f637c.

Required CI is complete and green, all threads are resolved, and I verified both round-1 Criticals against this head rather than against the resolved flags. The review that requested changes is bound to an older commit.

• R1-2, consent resolved from the wrong place: the workspace controller now takes one trust-gated loadSettings per call and shares it across the locale, the trust fallback and the telemetry options, and resolves consent through resolveUsageStatisticsEnabled, whose precedence (env flag, then the privacy setting, then true) is the same chain the session config uses. Standalone entrypoints that build no session Config now have one shared resolver to route through, which is the right shape for an opt-out that has to reach the logger.
• That shared load also carries skipLoadEnvironment, with the reason recorded: one daemon hosts every workspace, so writing the env of this workspace into the shared process environment would leak the values of one repo into the resolution of every other workspace.
• The proxy is scoped twice over. Only the resolved settings of this workspace may pick the telemetry proxy, because the ambient proxy env of the daemon is process-global and falling back to it would route the RUM uploads of this workspace through a proxy it never configured. And the throwaway Config is built with installProxyDispatcher false: the RUM logger pins its own agent from getProxy at upload time, so the proxy stays visible without installing a process-global undici dispatcher that nothing restores, which would otherwise re-route the plain fetch of the host process on every lifecycle event and leak the proxy of one workspace into every other workspace the daemon hosts.
• R1-1, the throwaway Config aborting the mutation it was built for: normalizeProxyUrl now runs inside a try and an unsupported value such as SOCKS is dropped to undefined, so the constructor cannot throw and the extension mutation always proceeds, degrading to the direct connection that was the behaviour before this PR. Telemetry losing a proxy is the right thing to sacrifice; a user-visible install or uninstall is not.
• The QwenLogger singleton limitation is disclosed rather than papered over: the docblock states that the logger binds to the first Config that opens its gate, so in a multi-workspace daemon the proxy of whichever workspace logs first wins for the process lifetime, and calls this pre-existing behaviour newly visible now that the proxy differs per workspace.

No new Critical found.

Re-reviewed at head 6981ff3d. Both historical Criticals are verifiably fixed in code — the throwaway telemetry Config no longer installs the process-global dispatcher (config.ts:3583, extensionManager.ts:601), SOCKS values can no longer abort an extension mutation (extensionManager.ts:586-591), and the daemon controller now resolves consent and proxy from the owning runtime's env with trust-gated, skipLoadEnvironment / consumeCorruptionEnvVars: false settings loads (workspace-extensions-controller.ts:328-362, :1168-1191). Wiring completeness re-verified: of the 12 non-test new ExtensionManager( sites, the 5 mutation-capable ones are wired and the 7 untouched ones cannot reach any lifecycle emission.

One discrepancy to clean up before the R2-6 thread is resolved: the closeout reply (16:50:48Z) says a new test does not let an untrusted workspace pick the status locale was added in 6981ff3dd8 and mutation-verified, but no such test exists at this head — the name appears nowhere in the tree, and workspace-extensions-controller.test.ts has no zh-CN assertion. The code fix itself is present and correct (workspace-extensions-controller.ts:1184-1191); only the pinning test for the untrusted re-load branch is missing. Could you either add the test as described or amend the reply so the thread state matches the tree?

The two items deliberately left open (R2-10/R2-11, split to #12852) and the documented first-writer-wins proxy note (R1-3) are fine as-is from my side.

CI at this head: all code checks green, including Test (ubuntu-latest) which carries the new suites; the only red is the review-pr automation job (review timeout, not a code signal).

在 head 6981ff3d 重新审过。两个历史 Critical 在代码层面均已确认修复——临时遥测 Config 不再安装进程全局 dispatcher（config.ts:3583、extensionManager.ts:601），SOCKS 值不会再中断扩展变更操作（extensionManager.ts:586-591），守护进程 controller 现在从所属 runtime 的 env 解析 consent 与 proxy，且 settings 加载带信任门控、skipLoadEnvironment 与 consumeCorruptionEnvVars: false（workspace-extensions-controller.ts:328-362、:1168-1191）。接线完整性也已复核：12 处非测试 new ExtensionManager( 中，5 处可产生变更的都已接线，未动的 7 处到不了任何生命周期事件发触点。

R2-6 线程在 resolve 前有一处出入需要清理：关闭回复（16:50:48Z）称 6981ff3dd8 新增了 does not let an untrusted workspace pick the status locale 测试并做过变异验证，但该测试在当前 head 不存在——整个代码树里搜不到这个用例名，workspace-extensions-controller.test.ts 里也没有任何 zh-CN 断言。代码修复本身已到位且正确（workspace-extensions-controller.ts:1184-1191），缺的只是不受信任分支重解析的钉住测试。麻烦补上该测试，或修正回复，让线程状态与实际代码树一致。

有意保持开放的两项（R2-10/R2-11，已拆到 #12852）以及已文档化的 first-writer-wins proxy 说明（R1-3），我这边没有异议。

当前 head 的 CI：所有代码检查全绿，包括承载新增测试套的 Test (ubuntu-latest)；唯一红色是 review-pr 自动化评审 job（评审超时，非代码信号）。

Review at head 24433f2e — approving (round 2)

Base e767e223. 19 files, +1175/-61 (scope-narrowed at head relative to earlier revisions). Privacy fix: extension lifecycle telemetry now honors usageStatisticsEnabled opt-out and routes uploads through the configured proxy.

Round 1 reviewed at d6f637c2 (same scope minus the daemon env isolation added since). Both historical Criticals (R1-1 dispatcher leak, R1-2 ambient env in daemon controller) were confirmed fixed at that head. This round re-verifies both at the current head and audits the new daemon env isolation logic.

What I checked at current head

R1-1 — process-global dispatcher leak. Verified fixed: extensionManager.ts passes installProxyDispatcher: false to the throwaway telemetry Config, and config.ts gates the setGlobalDispatcher + setResolvedProxyUrlForRuntimeFetch block on params.installProxyDispatcher ?? true. getProxy() still returns the value on that Config so QwenLogger.getProxyAgent() can pin its own agent — the proxy is routed correctly without installing a process-global side effect on every lifecycle event.

R1-2 — daemon controller ambient env. Verified fixed at current head. The new runtimeEnv path: workspaceDir === boundWorkspace && deps.env → snapshot { ...deps.env } in a try-catch (handles non-active runtime throwing on property access). The consentEnv construction is correctly one-directional: an ambient opt-out (QWEN_USAGE_STATISTICS_ENABLED=0/false) is preserved for any workspace the daemon hosts; an ambient opt-in is refused for non-bound workspaces (where the value may be contamination from another repo's settings load). This matches the documented intent — an operator's daemon-wide opt-out closes every gate, while a per-workspace opt-in is not inherited by other workspaces.

Proxy scoping. For a non-bound workspace or a non-active runtime: runtimeEnv ?? {} = {}, so resolveExtensionTelemetryProxy(settings.proxy, {}) returns settings.proxy only — no ambient proxy env leaks into that workspace's telemetry uploads. Correct.

normalizeProxyUrl try-catch. An unsupported proxy (SOCKS) drops to undefined rather than throwing and aborting the extension mutation. This is the correct tradeoff (telemetry falls back to direct; the mutation succeeds).

resolveUsageStatisticsEnabled shared resolver. The chain parseBooleanEnvFlag(env['QWEN_USAGE_STATISTICS_ENABLED']) ?? settingsValue ?? true matches the main session chain exactly. loadCliConfig now calls this helper, so the two cannot drift. Tested independently in config.test.ts.

CLI sites. install.ts, uninstall.ts, utils.ts all call resolveUsageStatisticsEnabled(settings.privacy?.usageStatisticsEnabled) and resolveExtensionTelemetryProxy(settings.proxy) with default env = process.env. For CLI commands this is correct — they are short-lived processes attributed to one workspace, and ambient env is their own.

Cross-check. Prior approvals from qqqys and chiga0 (me, round 1) at d6f637c2 confirmed both Criticals fixed. doudouOUC's review at 6981ff3dd8 confirmed the same and raised a discrepancy about a missing test for the untrusted-locale branch — that branch was subsequently moved to a separate PR (scope narrowed at 7d002fe42a), which resolves the discrepancy. Remaining open suggestions: R1-3 (QwenLogger singleton first-writer-wins proxy — disclosed in JSDoc, pre-existing limitation); R1-4 (proxy-chain duplication, tracked in follow-up #12857). Neither is a blocker.

No blocking findings. Approval blockers: none.

Checked: R1-1 dispatcher gate at current head; R1-2 daemon env isolation logic (runtimeEnv, consentEnv, one-directional narrowing); proxy scoping for non-bound workspaces; shared resolver chain; CLI wiring.
Not run: local vitest — CI green at this head.

Reviewed with AI assistance.

Reviewed at head 24433f2e49da1134653ecb8daeb7fae3ece774a4.

Verdict: APPROVE

Every Critical ever filed on this PR is fixed at this head — verified by reading the current code and its call chain, not by trusting thread-resolution flags — and an independent Critical-only scan of the diff found none.

Historical blockers, re-checked against this head

R1-1 — throwaway telemetry Config rewrote process-global network state and nothing restored it. Fixed. ConfigParameters gains installProxyDispatcher?: boolean, and the constructor's block is now gated: if (proxyUrl && (params.installProxyDispatcher ?? true)) before the dynamic import that calls setGlobalDispatcher(new EnvHttpProxyAgent(…)) and setResolvedProxyUrlForRuntimeFetch(proxyUrl). ExtensionManager.getTelemetryConfig passes installProxyDispatcher: false while still passing proxy, so getProxy() keeps feeding the RUM logger's own agent without the host process's plain fetch being re-routed on every lifecycle event. Default true leaves every session Config behaving exactly as before.

R1-2 — the workspace controller resolved consent and proxy from the daemon's ambient process.env, and a hoisted loadSettings wrote the workspace's .env into that shared env. Fixed on both halves. All three loadSettings sites in workspace-extensions-controller.ts now pass skipLoadEnvironment: true (:346/:348, :1238, :1257), so no workspace .env / settings.env reaches the daemon's shared env. And resolveUsageStatisticsEnabled no longer falls through to its env = process.env default: it receives consentEnv, which is runtimeEnv when the workspace in hand is the bound one, otherwise only the ambient opt-out term, otherwise {}. A hosted repo publishing QWEN_USAGE_STATISTICS_ENABLED=1 therefore cannot re-open the gate for a different workspace whose own settings opt out, while an operator's daemon-wide opt-out still closes every gate. Proxy is resolveExtensionTelemetryProxy(settings.proxy, runtimeEnv ?? {}) — settings first, and no ambient proxy for a workspace that never configured one.

R1-2 (fix-induced) — reading deps.env coupled every secondary workspace's extension routes to the primary runtime's lifecycle, since the delegate's get trap calls requirePrimaryRuntime() and throws once that entry leaves active. Fixed. runtimeEnv is built in an IIFE that returns undefined when workspaceDir !== boundWorkspace || !deps.env, and otherwise wraps { ...deps.env } in try/catch returning undefined. A property read on the live delegate can no longer throw out of a route serving a different workspace; the undefined then degrades to settings-only resolution rather than to the ambient env.

R2-1 (fix-induced) — the probe load parsed an untrusted workspace's own .qwen/settings.json, and parsing runs the migration / corruption-recovery path, which rewrites that file. Fixed. buildLocalExtensionsStatus now short-circuits the probe on an authoritative false from deps.isWorkspaceTrusted?.() (probeSettings = undefined), so an untrusted workspace performs exactly one load — the gated one, which passes skipWorkspaceSettings: true, workspaceTrusted: false. A trust-free, read-only-by-contract status poll reachable with a single GET can no longer mutate the workspace it reports on.

The Critical about the throwaway Config aborting an extension mutation on a proxy a session would reject at startup is also handled: normalizeProxyUrl(this.proxy) runs inside try/catch and an unsupported value drops to undefined, restoring the direct-connection behaviour rather than failing the install or uninstall.

Independent scan of the current diff

• The change is additive at the core boundary. ExtensionManagerOptions gains two optional fields, both stored and read only by the new private getTelemetryConfig. The module-level helper it replaces took telemetrySettings alone; existing constructions that pass neither field behave as before.
• The three CLI entrypoints are single-process, so ambient env is correct there. install.ts, uninstall.ts and utils.ts each hoist loadSettings(workspaceDir).merged into one settings const — removing a duplicate load, not adding one — and pass resolveUsageStatisticsEnabled(settings.privacy?.usageStatisticsEnabled) and resolveExtensionTelemetryProxy(settings.proxy). qwen extensions … exits before loadCliConfig, so the omitted leading CLI-flag proxy term genuinely cannot reach these handlers.
• loadCliConfig's rewrite is behaviour-preserving. resolveUsageStatisticsEnabled(settings.privacy?.usageStatisticsEnabled) with the default env = process.env expands to exactly the parseBooleanEnvFlag(…) ?? settings.privacy?.usageStatisticsEnabled ?? true chain it replaced.
• Env scoping is guarded twice, not once. workspace-extensions.ts forwards deps.env only when ws === boundWorkspace && deps.env, and the controller re-checks workspaceDir === boundWorkspace per manager it builds. A secondary workspace therefore resolves nothing from the primary runtime's env.
• installProxyDispatcher is not a dead switch. It is declared, read in the constructor's gate, and set to false by the one caller that needs it; no other construction site passes it, which is the intended default-true path.
• The disclosed residual is pre-existing and documented, not introduced here: QwenLogger is a process-wide singleton bound to the first Config that opens its opt-in gate, so in a multi-workspace daemon the first workspace's proxy wins for the process lifetime. The option comment says so plainly.

The three threads still open are all [Suggestion] — the ambient opt-out term being repository-writable (its effect is to close the gate, the fail-closed direction), the unpinned proxy precedence at the controller call site, and six of the seven converted getTelemetryConfig call sites having no witness asserting the Config argument. Per this channel's Critical-only policy they do not gate the verdict, and this PR is well past the review-round count at which suggestions defer to a follow-up.

I also note the review I submitted earlier on this PR was dismissed at d6f637c2ec; the head has moved 15 commits since, so nothing in this verdict relies on it — the invariants above were re-read at 24433f2e.

CI

At 24433f2e: 22 checks succeeded, 8 skipped, and only web-shell E2E Smoke and review-pr were still running, with no failures. Nothing attributable to this PR. Per the review policy, pending checks were not waited on.