fix(cli): expand ${VAR} placeholders in .mcp.json like every settings scope - #11703
bluefateludi wants to merge 5 commits into
Conversation
Fireworks' OpenAI-compatible endpoint accepts the non-standard reasoning_content field on input but rejects the additional reasoning field with HTTP 400 (Extra inputs are not permitted, field: 'messages[N].reasoning'). The default provider mirrors reasoning_content into reasoning for any model whose name contains 'qwen3' — a family match that does not establish the endpoint accepts the extra field — so Qwen3 thinking models on Fireworks fail every multi-turn continuation that replays a thinking turn, including tool-call continuation (issue QwenLM#11657). Add a Fireworks provider, detected by api.fireworks.ai hostname in determineProvider (following the Cerebras precedent from QwenLM#11049), that undoes the mirror at the outbound request boundary. Only the exact mirror copy (reasoning === reasoning_content) is dropped; a distinct explicit reasoning field and reasoning_content itself — which Fireworks documents for reasoning replay — are preserved. Conversation history is never mutated and non-Fireworks endpoints keep the existing mirroring.
… scope
A project .mcp.json keeping its secret in the environment sent the
placeholder text instead of the secret: headers like
'Authorization: Bearer ${MY_TOKEN}' reached the endpoint verbatim and
answered 401, while the byte-identical entry in .qwen/settings.json
worked — loadProjectMcpServers() never called the env resolver, and
assembleMcpServers() layers .mcp.json on top of the already-resolved
settings map, so nothing downstream could catch it either (issue
QwenLM#11499).
Resolve placeholders at load time, after Claude transport
normalization, with the same precedence settings scopes use
(process.env > home ~/.qwen/.env > unresolved placeholder) — the loader
resolves internally so every caller inherits the home fallback without
signature changes. The resolver's isInternalSecretEnvVar guard bounds
the untrusted-file angle exactly as it already is for workspace
settings and extensions.
Approval hashing: expansion would bind a stored approval to the
secret's value — rotating MY_TOKEN or a teammate cloning the repo with
their own token would flip an approved server back to pending although
the file never changed. QwenLM#4615's intent is that editing the file
re-triggers approval, so the loader also reports each server's
pre-expansion literal config, assembly registers it per project root,
and the approval store hashes that literal for project servers (the
live config remains the binding for settings-sourced servers, matching
existing behavior).
|
Qwen precheck requires maintainer approval before automated triage/review. Head SHA: Reason:
A maintainer with write access can inspect the PR and manually request a run with |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Blocking: a name collision between .mcp.json and workspace settings silently unbinds the #4615 approval gate
Reviewed at head a49d89211f5d5b65bc4791869a54c9fb6a8736ee.
The env-expansion half looks right to me, and binding a project server's approval to the pre-expansion literal is a sound idea. The problem is that the literal lookup is keyed by server name alone, while the server that actually executes may come from a different file.
Location
packages/cli/src/config/mcpApprovals.ts:36-43, consumed by getState (:200) and setState (:222), together with packages/cli/src/config/mcpServers.ts:50-55:
function approvalConfigHash(projectRoot, serverName, config) {
const literal = getProjectMcpLiteralSource(projectRoot)?.[serverName];
return hashMcpServerConfig(literal ?? config);
}Mechanism
assembleMcpServers merges {...belowProject, ...projectResult.servers, ...aboveProject, ...cliMcpServers}, so a settings entry with scope: 'workspace' (or 'system') overrides the same-named .mcp.json entry and is what runs. setProjectMcpLiteralSource(cwd, projectResult.literalServers) nevertheless stores the full .mcp.json literal map, including that overridden entry. approvalConfigHash then finds a literal for the name and returns its hash — with no scope or provenance check — so the executing workspace server's approval is bound to a config from a file it does not come from.
This is an explicitly supported configuration, not a misuse: packages/core/src/config/mcp-server-config.ts:12-16 documents that 'workspace' and 'system' rank above a .mcp.json, and isGatedMcpScope (:47-49) gates 'workspace' as well as 'project', so the overriding entry does go through this hash.
Trigger
A project shipping both .mcp.json and .qwen/settings.json that declare an MCP server under the same name. The user approves it once through the normal startup dialog.
Impact
Any later edit to the workspace entry — command, args, url, headers, i.e. precisely the fields configHash.ts calls behavioral — leaves getState returning 'approved', because the hash is computed from the unrelated .mcp.json literal and never changes. The server connects and executes a configuration the user never saw a prompt for. configHash.ts states that "Approval binding is security-sensitive", and this fails open in exactly that direction; #4615's invariant ("editing the file re-triggers approval") is silently defeated for the colliding server.
The inverse symptom also appears: editing the non-executing .mcp.json entry spuriously flips the workspace server back to pending.
Base arm
This is a regression introduced by this diff, not a pre-existing hole. On main, getState computes hashMcpServerConfig(config) over the live config, so any behavioral edit to the executing entry changes the digest and returns pending.
I verified the delta mechanically against the real hashMcpServerConfig from packages/core, with the head's approvalConfigHash and assembleMcpServers merge order, and with the source text of both arms asserted against the blobs rather than transcribed freehand:
| arm | after approve | after editing the executing workspace entry |
|---|---|---|
base (main) |
approved |
pending — gate holds |
| head (this PR) | approved |
approved — gate bypassed |
hash(approvalConfigHash) is identical across the edit on the head arm (true) and differs on the base arm (false), and the head's digest equals hashMcpServerConfig(projectLiteral) — confirming the binding is to the project literal. A negative control with a non-colliding name returns pending on both arms, so the harness is sensitive to the collision specifically rather than reporting the head as unconditionally broken.
Fix direction
Only consult the literal when the config being hashed genuinely came from .mcp.json. Either gate the lookup on provenance (config.scope === 'project'), or — more robustly — have assembly record literals only for the names that win the merge, so an overridden project entry never populates the map. The first is the smaller change; the second also removes the stale-entry class of bug for free.
Worth fixing in the same pass, though it is not itself blocking: the approvals file is keyed by normalizeProjectRoot(projectRoot) (path.resolve, lowercased on win32) while projectMcpLiteralSources is keyed by the raw cwd, so the two maps can disagree about identity. Today the miss direction is benign (it falls back to hashing the live config, i.e. re-prompts), but the asymmetry means whether this fix applies at all depends on the caller's spelling of the root.
Not covered by this comment
Everything else in the diff — the expansion precedence, the isInternalSecretEnvVar denylist, the literal/resolved pairing in loadProjectMcpServers, and the test additions — I read and did not find blocking. I did not review the openaiContentGenerator/provider/fireworks.* files, which appear to belong to #11701 and ride along because this branch is stacked on it; a maintainer may want this branch rebased so the two land independently.
中文摘要：approvalConfigHash 只按 服务器名 查 .mcp.json 的字面量，没有校验 scope 与来源。而 assembleMcpServers 中 scope: 'workspace'/'system' 的设置项会覆盖同名的 .mcp.json 条目并真正执行，字面量表却仍保存被覆盖的那一份；isGatedMcpScope 同样对 'workspace' 生效。于是当项目同时存在同名的 .mcp.json 与 .qwen/settings.json MCP 条目时，实际执行的 workspace 服务器的批准被绑定到了另一个文件的配置上——之后修改该 workspace 条目的 command/args/url/headers 都不会使 getState 回到 pending，#4615 的"改文件即重新批准"被静默绕过，且方向是 fail-open。main 上 getState 哈希的是实时配置，任何行为性修改都会重新要求批准，因此这是本 PR 引入的回归。修复方向：仅在配置确实来自 .mcp.json 时才使用字面量（按 config.scope === 'project' 判断，或只记录在合并中胜出的条目）。
|
Thanks @qqqys — confirmed and fixed in 39b7012.
Regression added in Also took the non-blocking root-key asymmetry in the same pass: the literal-source map is now keyed through the same |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Blocking: the new ./environment.js import breaks two ACP suites at collection time — QWEN_DIR is not in their core mock
Reviewed at head 39b701210aee6a525fcd363e7de3ec6b7e99d9d8. State read immediately before posting: state=open, merged=false, draft=false.
First: the Critical from my previous review is fixed — thank you
My earlier CHANGES_REQUESTED (review 5186310856, anchored at a49d89211f5d) reported the approval-hash name collision that unbound the #4615 gate for a workspace/system entry overriding a same-named .mcp.json server. That is resolved at this head, and I verified it against the blobs rather than the description:
mcpApprovals.ts:43-55— the literal lookup is now gated on provenance:config.scope === 'project' ? getProjectMcpLiteralSource(normalizeProjectRoot(projectRoot))?.[serverName] : undefined, thenhashMcpServerConfig(literal ?? config). A'workspace'/'system'override therefore hashes its own live config, so a behavioral edit to the executing entry re-prompts exactly as onmain, and editing the non-executing.mcp.jsonentry no longer flips it topending. Both symptoms I filed are closed.- The gate cannot silently disable literal binding for genuine project servers:
mcpJson.ts:126and:130stampscope: 'project'on the literal and the resolved entry, so the winning branch is the one that still consults the literal. - The root-key asymmetry is closed symmetrically —
literalSourceRootKey(mcpServers.ts:68-71,path.resolve+ win32 lowercase) is applied on both the write (:93) and the read (:107), matchingnormalizeProjectRoot. Double-normalizing at the call site is idempotent, so that is harmless. - Consumer set is complete:
getProjectMcpLiteralSourcehas exactly one production consumer (mcpApprovals.ts:50) andsetProjectMcpLiteralSourceexactly one production caller (mcpServers.ts:57). mcpApprovals.test.ts:741(does not bind a workspace-scope override to the same-named .mcp.json literal) asserts the right thing — registers the project literal under the colliding name, approves ascope: 'workspace'server, edits itshttpUrl, expectspending— and the neighbouring cases keep the project-scope side honest (rotation-stable, file-edit re-prompts, no-literal parity), so it is sensitive to the collision specifically.
That review row is anchored at a commit that is no longer head, so please read this one as superseding it.
New blocking issue
Location. packages/cli/src/config/mcpJson.ts:15, added by this PR:
import { getHomeEnvFallbackVars } from './environment.js';Mechanism. packages/cli/src/config/environment.ts:11 imports QWEN_DIR from @qwen-code/qwen-code-core and :31 reads it at module scope:
export const SETTINGS_DIRECTORY_NAME = QWEN_DIR;acpAgent.test.ts:248 (and acpAgent.worktree.test.ts:116) mock that package with an explicit allow-list object literal — async (importOriginal) => ({ SessionSourceService: …, INVOCATION_CONTEXT_META_KEY: …, … }) — which cherry-picks named exports and never spreads ...await importOriginal(). QWEN_DIR is not among them; it appears 0 times in all 32,468 lines of acpAgent.test.ts. So the moment mcpJson.ts enters the module graph, evaluating environment.ts:31 reads a name the mock does not define and vitest aborts collection:
Error: [vitest] No "QWEN_DIR" export is defined on the "@qwen-code/qwen-code-core" mock.
❯ src/config/environment.ts:31:40
31| export const SETTINGS_DIRECTORY_NAME = QWEN_DIR;
❯ src/config/mcpJson.ts:15:1
Trigger. Unconditional — no .mcp.json, no env var, no timing. Just collecting either suite.
Impact. Two previously-passing suites fail to collect in the Test (ubuntu-latest, Node 22.x) lane (job 103581922361), which is red at this head:
Failed Suites 2
FAIL src/acp-integration/acpAgent.test.ts
FAIL src/acp-integration/acpAgent.worktree.test.ts
Test Files 2 failed | 1044 passed (1046)
Tests 30183 passed | 88 skipped (30271)
They contribute 0 tests, so the ACP agent surface loses its whole regression net while the lane is red. This is a deterministic module-load error, not a flake — the lane runs vitest run --retry=2 and failed through the retries.
Base arm. main's mcpJson.ts does not import ./environment.js, so environment.ts is not in these suites' collection graph and both pass. Independent control: #11708 sits on the same base 3b2283ee0d3b, does not touch mcpJson.ts, and has Test (ubuntu-latest, Node 22.x) = success — so this is introduced here, not pre-existing on main.
Fix direction. Two reasonable options; the choice is yours:
- Smallest: add
QWEN_DIRto the core mock in both suites (one line each, matching howserve/workspace-service/__tests__/facade.test.tsalready does it). This unblocks CI immediately, but leaves the new module-scope coupling in place, so any future suite that mocks core with an allow-list and transitively importsmcpJson.tshits the same wall. Note that only one cli test in the repo listsQWEN_DIR:today, so there is no broad convention that would have caught this. - More robust: keep
environment.tsout ofmcpJson.ts's module graph — e.g. source the home-.envfallback from the@qwen-code/qwen-code-core/envVarResolverentry point already imported on line 14, or movegetHomeEnvFallbackVarsinto a module that does not evaluateQWEN_DIRat import time. A lazyawait import('./environment.js')would also work but forcesloadProjectMcpServersasync, which is a wider change than this PR needs.
One note so the two red lanes are not conflated
Lint & Static (ubuntu-latest, Node 22.x) is also red at this head, but not for anything in this diff — it is the lint-gate-freshness check reporting that eslint.config.js moved on main (1e768979c5a6, #11625) after this branch last incorporated it, and that lane checks out the branch head alone. Merging or rebasing current main clears it. Only the Test lane failure above is attributable to the change.
Scope of this comment
Everything else in the diff at this head — the expansion precedence, the isInternalSecretEnvVar denylist, the literal/resolved pairing in loadProjectMcpServers, and the approval-hash fix described above — I read and did not find blocking. As before, I did not review openaiContentGenerator/provider/fireworks.*, which belong to #11701 and ride along because this branch is stacked on it. This comment carries no approval and no merge recommendation; as of the state read immediately before posting there is no APPROVED review row from ci-bot or a maintainer on this PR, so the report/approve gate is not open on my side either.
中文摘要：先说好消息——上一轮 CHANGES_REQUESTED（review 5186310856，锚定在 a49d89211f5d）指出的批准哈希同名冲突已在当前 head 修复，我按 blob 逐条核对：mcpApprovals.ts:43-55 现在用 config.scope === 'project' 限定字面量查询，mcpJson.ts:126/130 确实给字面量与解析后的条目都打了 scope: 'project'（所以正常项目服务器仍走字面量绑定），literalSourceRootKey 在写入（mcpServers.ts:93）与读取（:107）两侧对称生效，root key 大小写不对称也一并修好了；mcpApprovals.test.ts:741 的回归测试断言方向正确，且相邻用例守住了 project 侧行为。那条 review 锚定的已不是 head，请以本条为准。
新的阻塞问题：本 PR 在 mcpJson.ts:15 新增了 import { getHomeEnvFallbackVars } from './environment.js'，而 environment.ts:31 在模块作用域读取 QWEN_DIR（来自 @qwen-code/qwen-code-core）。acpAgent.test.ts:248 与 acpAgent.worktree.test.ts:116 对该包的 mock 是显式白名单对象字面量（没有 ...await importOriginal() 展开），全文 32468 行里 QWEN_DIR 出现 0 次。于是 mcpJson.ts 一旦进入模块图，收集阶段就会抛 No "QWEN_DIR" export is defined on the mock，两个原本通过的 suite 直接收集失败：Test Files 2 failed | 1044 passed (1046)，Test (ubuntu-latest, Node 22.x) 变红（job 103581922361），ACP agent 这块失去全部回归覆盖。这是确定性的模块加载错误，不是 flake——该 lane 跑的是 vitest run --retry=2，重试后仍失败。基线侧：main 的 mcpJson.ts 不 import ./environment.js；同一 base 3b2283ee0d3b 上未改 mcpJson.ts 的 #11708 该 lane 为 success，可作为独立对照，说明是本 PR 引入。修复方向二选一：（1）最小改动——在两个 suite 的 core mock 里补上 QWEN_DIR（各一行，可参照 serve/workspace-service/__tests__/facade.test.ts），但模块作用域耦合仍在；（2）更稳——让 environment.ts 不进入 mcpJson.ts 的模块图，例如改从第 14 行已引入的 @qwen-code/qwen-code-core/envVarResolver 取该 fallback，或把 getHomeEnvFallbackVars 移到不在 import 期求值 QWEN_DIR 的模块。另外提醒：Lint & Static 也红，但与本 diff 无关——那是 lint-gate 新鲜度检查在提示 eslint.config.js 在 main 上（1e768979c5a6，#11625）已变动、而该 lane 只 checkout 分支 head，合并或 rebase 最新 main 即可消除；只有上面的 Test lane 失败归因于本次改动。本条评论不含 approve，也不构成合入建议；按发帖前即时读到的状态，本 PR 尚无 ci-bot 或 maintainer 的 APPROVED review，因此报告/approve 闸门在我这侧也未打开。
|
Thanks for the precise diagnosis — fixed in a73077b, along your option 2 (the more robust one). Fix. Why a copy rather than re-exporting from Verification.
Note on the branch head. Between the review and this fix, @shaojinwen merged |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
The blocking issue is fixed at a73077b9b39b — verified from head blobs and a green Test lane; I found nothing new that blocks
Re-reviewed at head a73077b9b39b0ecee13e566aae5c2dff363eac2f. Lifecycle read immediately before posting: state=open, merged=false, draft=false.
The Critical from review 5187616313 is resolved
My previous CHANGES_REQUESTED (review 5187616313, anchored at 39b701210aee) reported that the new import { getHomeEnvFallbackVars } from './environment.js' at mcpJson.ts:15 pulled environment.ts:31's module-scope export const SETTINGS_DIRECTORY_NAME = QWEN_DIR into the collection graph of acpAgent.test.ts and acpAgent.worktree.test.ts, whose allow-list core mocks never define QWEN_DIR, so both suites failed to collect and Test (ubuntu-latest, Node 22.x) went red.
You took option 2. I checked it against the head blobs rather than the description:
- Fixed at the mechanism, not just at the symptom.
mcpJson.ts:15now readsimport { getHomeEnvFallbackVars } from './home-env-fallback.js';(blobacad58ee14e8→7a1e99ac0e47) and./environment.jsis gone from the file's import list, soenvironment.tsis out of the graph. The part that makes this robust rather than local: the newhome-env-fallback.ts(59 lines, read in full) has no module-scope use of any core export —Storage.getGlobalQwenDir()andgetErrorMessage(e)are both inside the function body. An allow-list mock throws when a missing export is accessed, which is exactly why the original failure pointed atenvironment.ts:31:40and not at the import line, so importing this module cannot throw at collection time. - The two names it does use are safe in those suites anyway.
Storageis defined in both core mocks (acpAgent.test.ts:757,acpAgent.worktree.test.ts:213).getErrorMessageis not — 0 hits across 32,554 + 603 lines — but it is reached only inside thecatch, and neither suite mentionsloadProjectMcpServers,mcpJsonorhome-env-fallbackat all (0 hits), so that call site is never entered there. - The copy is faithful and
environment.tsis untouched.home-env-fallback.ts:26-58has the same candidate order, the sameQWEN_HOMEskip, the same??=first-wins and the sameonReadErrortext asenvironment.ts:222-254, andenvironment.ts's blob is60f21f6ae00bat both39b701210aeeand head, sosettings.ts/settings-cache.tssee no change. Your reasoning for a copy over a re-export is right — any import path throughenvironment.tskeeps the module-scope evaluation, which is the wall. Non-blocking and yours to call: two definitions of one resolver can drift, so if it is ever consolidated, pointingenvironment.tsat the new module keeps a single source. - The
mainmerge did not silently change what I had cleared.123d2e45ce8alanded between my review and this fix, so I compared blobs instead of trusting the diff:mcpApprovals.ts(2c4e7bb8fd15) andmcpServers.ts(5fc641216853) are byte-identical between39b701210aeeand head. OnlymcpJson.tschanged, andhome-env-fallback.tsis the sole addition. - Empirically.
Test (ubuntu-latest, Node 22.x)at head issuccess(job103618891196, 20:47:33Z → 21:27:42Z — a 40-minute run, not a skip), againstTest Files 2 failed | 1044 passed (1046)at39b701210aee.Lint & Staticissuccessas well: the merge cleared the freshness gate I had flagged as unrelated to your diff.
Coverage at this head
The diff is now 8 files — the stacked #11701 Fireworks files dropped out through the merge — of which 4 are production (home-env-fallback.ts new, mcpJson.ts, mcpApprovals.ts, mcpServers.ts) and 4 are tests. I read both production files the fix commit touches in full; the other two are blob-identical to the head my previous review cleared, where the expansion precedence, the isInternalSecretEnvVar denylist and the literal/resolved pairing in loadProjectMcpServers were already non-blocking. So no file in the current list is unreviewed, and I found no new blocking issue.
One thing worth recording because it is the part that usually gets lost in a move like this: mcpJson.test.ts contains no vi.mock at all, so falls back to the home ~/.qwen/.env for vars not in process.env drives the relocated function for real — QWEN_HOME pointed at a temp dir, MY_MCP_TOKEN=tok-from-home-env written into its .env, and loadProjectMcpServers asserted to produce Authorization: Bearer tok-from-home-env. The moved code is executed by a test in the lane that just went green, not merely relocated.
Posture, scoped to this read
As of the state read immediately before posting: this PR has 2 review rows, both mine, both CHANGES_REQUESTED, both anchored at commits that are no longer head (5186310856 @ a49d89211f5d, 5187616313 @ 39b701210aee); there is no APPROVED row from ci-bot or from a maintainer, and precheck-pr reports that maintainer approval is required before automated triage/review runs. So the report/approve gate is not open on my side, and this comment carries no approval and no merge recommendation. What it does record is that both findings those two rows report are resolved at a73077b9b39b, so neither row describes the PR as it now stands.
I have not dismissed them myself — withdrawing a blocking review is not something this review pass is authorized to do — so I am flagging it instead: mergeable_state reads blocked at this head solely because of those two stale rows, and a maintainer who agrees they are stale can dismiss them.
中文摘要：在 head a73077b9b39b0ecee13e566aae5c2dff363eac2f 复核（发帖前即时读到 state=open、merged=false、draft=false）。上一轮 CHANGES_REQUESTED（review 5187616313，锚定 39b701210aee）指出的阻塞问题已修复，我按 head blob 逐条核对而非依赖描述：mcpJson.ts:15 已改为 from './home-env-fallback.js'（blob acad58ee14e8 → 7a1e99ac0e47），./environment.js 已从该文件 import 列表消失，environment.ts 不再进入模块图；更关键的是新模块 home-env-fallback.ts（59 行，全文读过）在模块作用域不使用任何 core 导出——Storage.getGlobalQwenDir() 与 getErrorMessage(e) 都在函数体内，而白名单 mock 是在访问缺失导出时才抛错（这正是当初报错指向 environment.ts:31:40 而非 import 行的原因），所以 import 该模块不可能在收集期抛错。两个 suite 的 core mock 本身也确实定义了 Storage（acpAgent.test.ts:757、acpAgent.worktree.test.ts:213）；getErrorMessage 没有（32554 + 603 行内 0 命中），但它只在 catch 内被访问，而两个 suite 完全没有引用 loadProjectMcpServers / mcpJson / home-env-fallback（0 命中），因此那条路径不会被进入。副本与原函数一致（候选顺序、QWEN_HOME 跳过、??= 先到先得、onReadError 文案均相同），且 environment.ts 的 blob 在 39b701210aee 与 head 同为 60f21f6ae00b，settings.ts / settings-cache.ts 不受影响；你选择复制而非 re-export 的理由成立（任何经过 environment.ts 的 import 路径都会保留模块作用域求值）。非阻塞一点：同一解析器存在两份定义、日后可能漂移，若要合并，让 environment.ts 反向 import 新模块即可保持单一来源。另外 123d2e45ce8a 把 main 并入分支，所以我用 blob 对比而不是看 diff：mcpApprovals.ts（2c4e7bb8fd15）与 mcpServers.ts（5fc641216853）在两个 head 之间逐字节相同，说明合并未悄悄改动我此前已放行的代码。实测侧：head 上 Test (ubuntu-latest, Node 22.x) 为 success（job 103618891196，20:47:33Z → 21:27:42Z，是 40 分钟的真实运行而非 skip），而 39b701210aee 上是 Test Files 2 failed | 1044 passed (1046)；Lint & Static 也已转绿——正如你所说，合并 main 清掉了我标注为与本 diff 无关的新鲜度门槛。
当前 head 的覆盖情况：diff 现为 8 个文件（叠在 #11701 上的 Fireworks 文件因合并而退出 diff），其中 4 个生产文件（新增 home-env-fallback.ts、mcpJson.ts、mcpApprovals.ts、mcpServers.ts）、4 个测试文件。本次 fix commit 触及的两个生产文件我全文读过，另两个与我上一轮已放行的 head 逐字节相同（当时已认定展开优先级、isInternalSecretEnvVar 拒绝名单、loadProjectMcpServers 中字面量/解析结果配对均不阻塞），因此当前文件清单没有未审阅项，也没有发现新的阻塞问题。值得记录的一点：mcpJson.test.ts 完全没有 vi.mock，所以 falls back to the home ~/.qwen/.env for vars not in process.env 这条用例是真实驱动被搬移的函数（QWEN_HOME 指向临时目录、写入 MY_MCP_TOKEN=tok-from-home-env、断言 loadProjectMcpServers 产出 Authorization: Bearer tok-from-home-env），即搬移后的代码在刚刚转绿的 lane 里被测试执行，而不只是换了位置。
发帖前即时读到的状态：本 PR 共 2 条 review，都是我提交的 CHANGES_REQUESTED，且都锚定在已非 head 的提交上（5186310856 @ a49d89211f5d、5187616313 @ 39b701210aee）；没有来自 ci-bot 或 maintainer 的 APPROVED，且 precheck-pr 显示需要 maintainer 批准后才会跑自动 triage/review。因此报告/approve 闸门在我这侧未打开，本条评论不含 approve，也不构成合入建议；它记录的是这两条 review 所指的问题在 a73077b9b39b 均已解决，因此两条都已不再描述当前 PR。我没有自行 dismiss——撤回阻塞性 review 超出本次 review 的授权范围——所以在此明确标出：当前 head 的 mergeable_state 之所以是 blocked，只因这两条已过期的 review；若 maintainer 认同其已过期，可以 dismiss。
|
Thank you for the thorough re-review — and for checking the fix against the head blobs and the actual 40-minute Test lane rather than the description; the note that On the non-blocking point: agreed that two copies of one resolver can drift. We've left Also noting for whoever holds the dismiss button: per your read, both |
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Traced the expansion and the approval binding at the PR head. No blocking issues found.
- The expansion applies the same precedence as the settings scopes (process.env → home
.env→ unresolved placeholder), runs after Claude-shape normalization so both the raw and normalized field names are covered, and the literal record keeps the same normalized-but-unresolved shape, so the two configs differ only in expansion — which is exactly what the approval hash needs to stay stable across secret rotation. - The binding is correctly scoped:
approvalConfigHashconsults the literal only whenconfig.scope === 'project', so a settings-scoped override of a same-named.mcp.jsonentry keeps binding to the executing config — a settings server can never silently ride on a literal from a file it did not come from. The root key folds win32 drive-letter case the same waymcpApprovals'normalizeProjectRootdoes, so a differently-cased caller cannot miss the literal. resolveEnvVarsInObjectstill refuses Qwen-internal secrets (isInternalSecretEnvVar), so the untrusted-file angle is bounded the same way it already is for workspace settings, and expansion happens at load time without spawning or connecting — the approval gate (#4615) is untouched.home-env-fallback.tsis deliberately a standalone copy (the comment says why: importing the settings-environment module would evaluateQWEN_DIRat module scope and break allow-listed core mocks), and its??=first-wins plus!Object.hasOwn(process.env, key)matches the documented precedence.- The core
./envVarResolversubpath is a real export-map entry (packages/core/package.json), not a deep import, so this does not depend on dist internals.
6 participants
What this PR does
Makes a project
.mcp.jsonexpand$VAR/${VAR}placeholders in server entries (headers, env, url, httpUrl, command, args) exactly like every settings scope already does, and keeps approval decisions bound to the file's literal text rather than the resolved secret.Two coordinated changes:
.mcp.jsonloader resolves placeholders at load time, after Claude transport normalization, using the same precedence as settings scopes:process.env> home~/.qwen/.env> unresolved placeholder. The loader resolves internally, so every caller (assembly, hot-reload, themcpcommands) inherits the home-.envfallback without signature changes — a token in~/.qwen/.envnow works identically in.mcp.jsonand.qwen/settings.json.Why it's needed
A checked-in
.mcp.jsonthat keeps its secret in the environment is the only safe way to configure an authenticated server, but today the placeholder text is sent verbatim:{"Authorization": "Bearer ${MY_TOKEN}"}withMY_TOKENset in the environment reaches the endpoint as the literal string${MY_TOKEN}and answers 401, so the server shows as Disconnected with no hint at the cause. The byte-identical entry in.qwen/settings.jsonworks, because settings scopes run the env resolver and the.mcp.jsonpath never does —assembleMcpServers()layers.mcp.jsonon top of the already-resolved settings map, so nothing downstream could catch it either (#11499; settings-scope resolution itself was #4466/#4474).The approval-hash half closes the side effect the triage note flagged: once expansion happens at load time, hashing the live config would bind a stored approval to the secret's value — rotating
MY_TOKEN, or a teammate cloning the repo with their own token, would flip an approved server back topendingand re-prompt, although the file never changed. #4615's intent is that editing the file re-triggers approval. Binding to the pre-expansion literal keeps approvals stable across environment changes while a real file edit still reverts the server topending. (Workspace.qwen/settings.jsonis already in the value-bound position today; this PR does not change that — noted as an existing wart, deliberately not widened or fixed here.)The untrusted-file angle is unchanged:
resolveEnvVarsInString()already refuses Qwen-internal secrets (isInternalSecretEnvVar), bounding.mcp.jsonexactly as workspace settings and extensions already are, and the approval dialog /ServerDetailStepnever renderheaders/env, so expansion puts no secret on screen.Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/config/mcpJson.test.ts src/config/mcpServers.test.ts src/config/mcpApprovals.test.ts src/commands/mcp/approve.test.ts src/config/hot-reload.test.ts src/commands/mcp/list.test.ts— 111 tests, all green (16 new). Onmain, the new expansion tests fail (headers keep the literal${MY_TOKEN}).Behavior to confirm, per the issue's reproduction: a
.mcp.jsonwith"headers": {"Authorization": "Bearer ${MY_TOKEN}"}andMY_TOKENin the environment now receives the real token at the endpoint; the same token defined only in~/.qwen/.envalso resolves; an undefined variable stays literal (no error, no empty substitution); a Claude-style{"type": "http", "url": "https://host/${VAR}/mcp"}expands after transport normalization (the placeholder travels intohttpUrl).Approval binding: approve a server, then change only the env value behind its placeholder — the approval holds (
approved, no re-prompt); edit the.mcp.jsonentry itself — the server returns topending. Both pinned by unit tests.Mutation checks: disabling the expansion (return the literal config) turns exactly the 4 expansion tests red; binding the hash to the live config instead of the literal turns exactly the rotation-stability test red — the suite pins both halves of the change.
Evidence (Before & After)
Before (
main):.mcp.jsonBearer ${MY_TOKEN}→ endpoint receivesBearer ${MY_TOKEN}→ 401 (issue report); new tests red.After (this branch, commit a49d892):
Tested on
Environment (optional)
vitest unit tests on Windows x64, Node v22, npm workspaces install.
Risk & Scope
.qwen/settings.jsonremains value-bound in approvals (pre-existing, unchanged); making.mcp.jsonresolution participate in settings hot-reload env precedence is untouched.Linked Issues
Fixes #11499
中文说明
本 PR 做了什么
让项目
.mcp.json中的$VAR/${VAR}占位符（headers、env、url、httpUrl、command、args）像所有 settings 作用域一样展开，并让审批决定绑定到文件的字面文本而非展开后的密钥值。两个配套改动：
.mcp.jsonloader 在加载时、Claude 传输形态归一化之后展开占位符，优先级与 settings 作用域一致：process.env> home~/.qwen/.env> 未解析占位符保持原样。由 loader 内部展开，所有调用方（组装、热重载、mcp 命令）无需改签名即继承 home.env回退 —— 放在~/.qwen/.env的 token 现在在.mcp.json和.qwen/settings.json里行为一致。为什么需要
把密钥放在环境变量里、
.mcp.json按名引用，是配置带认证 server 的唯一安全方式；但现在占位符文本被原样发送：{"Authorization": "Bearer ${MY_TOKEN}"}在MY_TOKEN已设置的情况下仍以字面量${MY_TOKEN}到达端点并得到 401，server 显示 Disconnected 且毫无线索。字节相同的条目放在.qwen/settings.json却能工作 —— 因为 settings 作用域跑了 env 解析器而.mcp.json路径从未跑过，且assembleMcpServers()把.mcp.json叠加在已解析的 settings 之上，下游也无法补救（#11499；settings 作用域的解析本身来自 #4466/#4474）。审批哈希一半是三角旗备注指出的副作用：一旦加载期展开，哈希运行时配置会把已存审批绑到密钥的值上 —— 轮换
MY_TOKEN或同事用自己的 token 克隆仓库都会让已批准的 server 回到pending重新弹窗，尽管文件从未改过。#4615 的本意是改文件才触发重新审批。绑定展开前字面配置让审批在环境变化下保持稳定，真实文件编辑仍会回到pending。（workspace.qwen/settings.json今天就已处于按值绑定状态；本 PR 不改变它 —— 作为既有瑕疵记录，刻意不在本 PR 扩大或修复。）不可信文件的角度没有变化：
resolveEnvVarsInString()已拒绝 Qwen 内部密钥（isInternalSecretEnvVar），.mcp.json的边界与 workspace settings、extension 完全一致；审批弹窗与ServerDetailStep从不渲染headers/env，展开不会把密钥带上屏幕。审阅者测试计划
如何验证
cd packages/cli && npx vitest run src/config/mcpJson.test.ts src/config/mcpServers.test.ts src/config/mcpApprovals.test.ts src/commands/mcp/approve.test.ts src/config/hot-reload.test.ts src/commands/mcp/list.test.ts—— 111 个测试全绿（新增 16 个）。在main上新增的展开测试为红（headers 保持字面${MY_TOKEN}）。按 issue 复现步骤确认的行为：
.mcp.json写"headers": {"Authorization": "Bearer ${MY_TOKEN}"}且环境有MY_TOKEN时端点收到真实 token；只定义在~/.qwen/.env的 token 同样展开；未定义变量保持字面（不报错、不替换为空）；Claude 形态{"type": "http", "url": "https://host/${VAR}/mcp"}在传输归一化后展开（占位符随httpUrl携带）。审批绑定：批准一个 server 后仅改占位符背后的环境值 —— 审批保持（
approved，不重新弹窗）；编辑.mcp.json条目本身 —— server 回到pending。两者均有单元测试钉住。变异验证：关闭展开（返回字面配置）恰好使 4 个展开测试变红；把哈希绑定改为运行时配置恰好使轮换稳定性测试变红 —— 测试套件同时钉住改动的两半。
证据（改前/改后）
改前（
main）：.mcp.json的Bearer ${MY_TOKEN}→ 端点收到Bearer ${MY_TOKEN}→ 401（issue 报告）；新增测试为红。改后（本分支，提交 a49d892）：
测试平台
环境（可选）
Windows x64 上的 vitest 单元测试，Node v22，npm workspaces 安装。
风险与范围
.qwen/settings.json的审批仍是按值绑定（既有行为，未改）；.mcp.json解析参与 settings 热重载的环境优先级不在本 PR 范围。关联 Issue
Fixes #11499