fix(core): parse QWEN_SERVE_MCP_CLIENT_BUDGET strictly as a decimal integer - #5752
Conversation
…nteger
readBudgetFromEnv used Number(rawBudget), so Number("0x10")=16, Number("1e2")=100
and Number("1.0")=1 all passed isInteger and were silently accepted as a budget.
Require plain decimal digits (/^\d+$/) like the other integer env vars, keeping
the existing stderr breadcrumb for invalid values.
|
@qwen-code /triage |
|
Thanks for the PR, @he-yufeng! Template has the essential sections (What, Why, Reviewer Test Plan, Risk) ✓ On direction: this closes the last gap in the strict-decimal integer-env parsing series. PRs #5602, #5612, and #5679 (all merged June 22–23) established the pattern for API timeout, serve rate limit, and agent/workflow env vars. This PR brings the two MCP budget parsing paths into alignment. CHANGELOG 0.19.1 lists the prior three strict-parsing fixes — this is the natural completion. Clearly aligned. On approach: minimal and focused — two parsing sites get the same Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向：这是严格十进制整数 env 解析系列的最后一块。#5602、#5612、#5679 已在 6 月 22–23 日合并，分别覆盖了 API 超时、serve 速率限制和 agent/workflow env 变量。CHANGELOG 0.19.1 记录了前三个严格解析修复——本 PR 是自然的收尾。方向明确对齐。 方案：极简且聚焦。两个解析点统一使用 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewThe fix is correct and consistent. Both parsing sites now use the identical strict pipeline:
This rejects Key things verified:
No correctness bugs, no security concerns, no over-abstraction. Reuse check: the strict-parsing pattern already exists in Unit TestsBoth focused test suites pass with zero regressions: The CLI test stderr output confirms all invalid values produce the breadcrumb: Tmux TestingN/A — this is a non-TUI env-parsing fix. The behavior change is purely internal (rejecting malformed env values that were previously silently accepted). Unit tests fully cover both parsing paths with comprehensive edge cases. 中文说明代码审查修复正确且一致。两个解析点现在使用完全相同的严格管线：
拒绝了 关键确认：
无正确性 bug，无安全隐患，无过度抽象。复用检查：严格解析模式已存在于 单元测试两套聚焦测试均通过，零回归：
CLI 测试的 stderr 输出确认所有非法值都产生了提示信息。 Tmux 测试不适用——这是非 TUI 的 env 解析修复。行为变化在内部（拒绝之前被静默接受的非法 env 值）。单元测试已完全覆盖两条解析路径。 — Qwen Code · qwen3.7-max |
|
This is a clean finishing PR. The codebase already committed to strict decimal-env parsing in three earlier PRs (#5602, #5612, #5679); this one closes the two remaining MCP budget sites with the same proven pattern. The diff is +86 / −21 across 4 files — exactly what the goal requires, nothing more. My independent proposal was: add a Both parsing paths are now byte-identical in their validation logic, both have dedicated test coverage (17 test cases total covering accept and reject paths), and all tests pass. The author's follow-up commits (7ce2a88 addressing the No reservations. Ships the fix cleanly. ✅ 中文说明这是一个干净的收尾 PR。代码库已在三个早期 PR（#5602、#5612、#5679）中承诺了严格十进制 env 解析；本 PR 用同样的已验证模式补上剩余的两个 MCP budget 入口。diff 为 +86 / −21，跨 4 个文件——恰好覆盖目标，没有多余内容。 我的独立方案是：在 两条解析路径的验证逻辑完全一致，都有专项测试覆盖（共 17 个测试用例覆盖接受和拒绝路径），所有测试通过。作者的后续提交（7ce2a886 修复 无顾虑。干净地交付修复。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
[Critical] createWorkspaceMcpBudget in packages/cli/src/acp-integration/acpAgent.ts:2497-2518 still parses QWEN_SERVE_MCP_CLIENT_BUDGET with the old loose Number(rawBudget) + Number.isInteger pattern. The comment on line 2499 says "Match McpClientManager.readBudgetFromEnv's parsing exactly" — but that invariant is now broken.
With QWEN_SERVE_MCP_CLIENT_BUDGET=0x10: readBudgetFromEnv rejects it (budget=undefined), but createWorkspaceMcpBudget accepts it as 16. The two enforcement layers disagree.
Fix: extract a shared parsing helper (or reuse parsePositiveIntegerEnv from packages/core/src/utils/env.ts) and call it from both sites.
— qwen3.7-max via Qwen Code /review
| // and Number('1.0')=1 all pass isInteger, so a loose parse would silently | ||
| // accept them. Only plain decimal digits should set a budget. | ||
| const trimmed = rawBudget.trim(); | ||
| const parsed = Number(trimmed); |
There was a problem hiding this comment.
[Suggestion] The inline parsing logic (trim() → /^\d+$/ → Number.isSafeInteger → > 0) is a reimplementation of the existing parsePositiveIntegerEnv utility in packages/core/src/utils/env.ts (same package). If validation rules change, all copies must be updated in lockstep.
Consider replacing the inline parse with:
const budget = parsePositiveIntegerEnv('QWEN_SERVE_MCP_CLIENT_BUDGET');The stderr warning branch still works since undefined maps to the else path.
— qwen3.7-max via Qwen Code /review
| try { | ||
| for (const bad of ['0x10', '1e2', '1.0']) { | ||
| process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'] = bad; | ||
| const manager = mkManager({ config: configWithServers({}) }); |
There was a problem hiding this comment.
[Suggestion] writeSpy is created on process.stderr.write but never asserted on. The adjacent pre-existing test (line 2609) follows the same setup pattern but does assert: expect(calls.some(s => s.includes('ignoring invalid ...'))).toBe(true).
The stderr breadcrumb is the sole operator-facing signal for rejected values. Add an assertion after the rejection loop:
const calls = writeSpy.mock.calls.map((c) => String(c[0]));
expect(calls.some(s => s.includes("'0x10'"))).toBe(true);— qwen3.7-max via Qwen Code /review
✅ Local runtime verification (real
|
QWEN_SERVE_MCP_CLIENT_BUDGET |
BASE (loose Number()) |
PR (strict /^\d+$/ + isSafeInteger) |
|
|---|---|---|---|
0x10 (hex) |
accepted (→16) | rejected | ⬅ fixed |
1e2 (scientific) |
accepted (→100) | rejected | ⬅ fixed |
1.0 (float) |
accepted (→1) | rejected | ⬅ fixed |
16.0 (float) |
accepted (→16) | rejected | ⬅ fixed |
99999999999999999999 (unsafe int) |
accepted | rejected | ⬅ fixed (bonus) |
abc |
rejected | rejected | control — unchanged |
0 / -5 |
rejected | rejected | control — unchanged |
16 / 100 (valid) |
accepted | accepted | control — unchanged |
So the change is targeted: the five previously‑silently‑accepted malformed forms (hex / scientific / float / unsafe‑large) now get rejected with the operator breadcrumb, while valid decimals still work and values that were already rejected are untouched. The 99999999999999999999 row is a nice extra — base's Number.isInteger(1e20) passed, so it accepted a precision‑losing budget; the PR's Number.isSafeInteger closes that too.
Real TUI confirmation (same bundle, interactive): …=0x10 → breadcrumb on stderr (rejected); …=16 → no breadcrumb (accepted).
Unit‑level A/B (resolved value, authoritative)
packages/core/src/tools/mcp-client-manager.test.ts -t "readBudgetFromEnv rejects non-decimal"
- PR source → ✅ passes (
getMcpClientBudget()isundefinedfor0x10/1e2/1.0). - Base source (file reverted to merge‑base) → ❌ fails:
AssertionError: expected 16 to be undefined— proving base silently resolved0x10to a budget of 16. - Full
mcp-client-manager.test.tson PR: 102/102 pass (no regression).
Notes
- This parser runs in the ACP child / standalone
qwenpath (the CLI--mcp-client-budgetflag is separately validated inrun-qwen-serve.tsand unaffected). Verified via realqwenstartup where the McpClientManager constructor callsreadBudgetFromEnv(). - The existing stderr breadcrumb still fires for every rejected value (confirmed live).
Verdict
Behaves exactly as described, in real qwen processes and at the unit level, with a clean A/B against base and no regression. The fix also closes an adjacent unsafe‑integer hole. LGTM. ✅
中文版（合并参考）
✅ 本地真实运行时验证（真实 qwen 进程，PR vs base 的 A/B）
我从本 PR 的最新 head（162c38d11）构建了两个真实 bundle——一个含修复，另一个仅把 mcp-client-manager.ts 回退到 merge-base——并通过真实的 qwen 运行（headless -p 与交互式 TUI）外加单元级 A/B，验证了 QWEN_SERVE_MCP_CLIENT_BUDGET 的解析。可观察信号是 breadcrumb qwen serve: ignoring invalid QWEN_SERVE_MCP_CLIENT_BUDGET=…：出现=被拒绝，不出现=被当作 budget 接受。
运行时 A/B 矩阵（每格一个真实 qwen 进程，相同环境，两个 bundle）
QWEN_SERVE_MCP_CLIENT_BUDGET |
BASE（宽松 Number()） |
PR（严格 /^\d+$/ + isSafeInteger） |
|
|---|---|---|---|
0x10（十六进制） |
接受（→16） | 拒绝 | ⬅ 已修复 |
1e2（科学计数） |
接受（→100） | 拒绝 | ⬅ 已修复 |
1.0（浮点） |
接受（→1） | 拒绝 | ⬅ 已修复 |
16.0（浮点） |
接受（→16） | 拒绝 | ⬅ 已修复 |
99999999999999999999（不安全整数） |
接受 | 拒绝 | ⬅ 已修复（额外收益） |
abc |
拒绝 | 拒绝 | 对照——不变 |
0 / -5 |
拒绝 | 拒绝 | 对照——不变 |
16 / 100（合法） |
接受 | 接受 | 对照——不变 |
可见改动是精准的：此前被静默接受的五类畸形值（十六进制/科学计数/浮点/超大不安全整数）现在都会被拒绝并打印 breadcrumb；合法十进制仍正常工作；本就被拒绝的值不受影响。99999999999999999999 这一行是额外亮点——base 的 Number.isInteger(1e20) 为真，于是接受了一个会丢精度的 budget，而 PR 的 Number.isSafeInteger 也把它堵上了。
真实 TUI 佐证（同一 bundle，交互式）：…=0x10 → stderr 出现 breadcrumb（拒绝）；…=16 → 无 breadcrumb（接受）。
单元级 A/B（解析后的实际取值，权威）
packages/core/src/tools/mcp-client-manager.test.ts -t "readBudgetFromEnv rejects non-decimal"
- PR 源码 → ✅ 通过（
0x10/1e2/1.0时getMcpClientBudget()为undefined）。 - Base 源码（该文件回退到 merge-base）→ ❌ 失败：
AssertionError: expected 16 to be undefined——证明 base 把0x10静默解析成了 budget 16。 - PR 上完整跑
mcp-client-manager.test.ts：102/102 通过（无回归）。
说明
- 该解析运行在 ACP child / 独立
qwen路径（CLI 的--mcp-client-budgetflag 在run-qwen-serve.ts中另有校验，不受影响）。通过真实qwen启动时 McpClientManager 构造函数调用readBudgetFromEnv()得到验证。 - 既有的 stderr breadcrumb 对每个被拒绝的值仍会触发（已实机确认）。
结论
在真实 qwen 进程与单元层面，行为均与描述完全一致；与 base 的 A/B 干净、无回归，并顺带堵上了相邻的不安全整数漏洞。LGTM。 ✅
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
[Critical] createWorkspaceMcpBudget in packages/cli/src/acp-integration/acpAgent.ts:2497-2518 still parses QWEN_SERVE_MCP_CLIENT_BUDGET with loose Number(rawBudget) + Number.isInteger — no regex, no trim, no isSafeInteger. The comment at line 2497 says "Match McpClientManager.readBudgetFromEnv's parsing exactly" but is now false after this PR.
The pool (acpAgent.ts) and per-client manager (readBudgetFromEnv) silently disagree: QWEN_SERVE_MCP_CLIENT_BUDGET=0x10 is rejected by the manager (no budget) but accepted by the pool (budget=16). Fix: apply the same /^\d+$/ + isSafeInteger + trim() pattern, or extract a shared utility.
Also: CLI-layer validation in serve.ts:343-345 and run-qwen-serve.ts:656-658 still uses isInteger not isSafeInteger — narrow inconsistency but the three validation sites should agree.
— qwen3.7-max via Qwen Code /review
| }); | ||
|
|
||
| it('readBudgetFromEnv rejects non-decimal budget values (hex / scientific / float)', async () => { | ||
| const writeSpy = vi.spyOn(process.stderr, 'write').mockReturnValue(true); |
There was a problem hiding this comment.
[Suggestion] The .trim() code path added at source line 284 is not tested. Consider adding whitespace-padded values: " 16 " (should be accepted as 16) and " 0x10 " (should still be rejected after trim) to the test cases.
— qwen3.7-max via Qwen Code /review
| // a plain decimal integer is still accepted. | ||
| process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'] = '16'; | ||
| const ok = mkManager({ config: configWithServers({}) }); | ||
| expect(ok.getMcpClientBudget()).toBe(16); |
There was a problem hiding this comment.
[Suggestion] The upgrade from isInteger to isSafeInteger tightens the upper bound, but no test exercises the boundary. Consider adding "9007199254740992" (MAX_SAFE_INTEGER + 1, passes /^\d+$/ but fails isSafeInteger) to the rejection loop, and "9007199254740991" to the acceptance assertions to bracket the boundary.
— qwen3.7-max via Qwen Code /review
The pooled path parsed QWEN_SERVE_MCP_CLIENT_BUDGET with a loose Number() + isInteger, so 0x10, 1e2 and 1.0 silently set a budget while the manager's readBudgetFromEnv (already strict) rejected them. Use the same /^\d+$/ + isSafeInteger parse and emit the same stderr breadcrumb on invalid input. Export the function and cover the parsing with unit tests.
|
Good catch, thanks — fixed in 7ce2a88.
I exported the function and added unit tests in |
|
@qwen-code /review |
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/28029781007)._ |
| // Match `McpClientManager.readBudgetFromEnv`'s parsing exactly. | ||
| // Use `Number(...)` + `Number.isInteger` so the pool and the manager | ||
| // Match `McpClientManager.readBudgetFromEnv`'s parsing exactly: only plain | ||
| // decimal digits set a budget. A loose `Number(...)` would silently accept |
There was a problem hiding this comment.
[Suggestion] The comment says "Match McpClientManager.readBudgetFromEnv's parsing exactly" but this is only true for the budget value parsing. The mode handling diverges in two observable ways:
readBudgetFromEnvemits a stderr breadcrumb whenQWEN_SERVE_MCP_BUDGET_MODEis set to an unrecognized value (e.g.ENFORCE);createWorkspaceMcpBudgetsilently falls through to the default.readBudgetFromEnvemits a stderr breadcrumb when mode isenforce/warnbut no budget is set (downgrading tooff);createWorkspaceMcpBudgetreturnsundefinedsilently.
A future maintainer reading "exactly" may skip checking readBudgetFromEnv for features to replicate here. Consider scoping the claim:
| // decimal digits set a budget. A loose `Number(...)` would silently accept | |
| // Match `McpClientManager.readBudgetFromEnv`'s budget-value parsing: |
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it('accepts a plain positive decimal integer', () => { |
There was a problem hiding this comment.
[Suggestion] MODE is declared and cleaned up in afterEach but never set in any test case. The mode-resolution IIFE has four branches (enforce, warn, off, and the fallback budget !== undefined ? 'warn' : 'off') — all untested. Consider adding at least:
it('respects an explicit budget mode', () => {
process.env[KEY] = '10';
process.env[MODE] = 'enforce';
// ...assert mode is 'enforce'
});
it('returns undefined for mode=off even with a budget', () => {
process.env[KEY] = '10';
process.env[MODE] = 'off';
expect(createWorkspaceMcpBudget(onEvent)).toBeUndefined();
});| }); | ||
|
|
||
| it('accepts a plain positive decimal integer', () => { | ||
| process.env[KEY] = '100'; |
There was a problem hiding this comment.
[Suggestion] The "accepts" tests only assert toBeDefined(), which verifies the constructor was reached but not what was passed to it. The WorkspaceMcpBudget mock discards constructor arguments, so a regression that passes a wrong budget value or wrong default mode would go undetected.
Compare with the core test which asserts the actual parsed value (expect(ok.getMcpClientBudget()).toBe(16)). Since WorkspaceMcpBudget is a vi.fn(), you could assert on constructor call args:
const MockBudget = vi.mocked(WorkspaceMcpBudget);
createWorkspaceMcpBudget(onEvent);
expect(MockBudget).toHaveBeenCalledWith(
expect.objectContaining({ clientBudget: 100, mode: 'warn' }),
);
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Critical recheck: the loose MCP budget parsing issue is fixed on the current head. Both the core MCP manager and ACP workspace child helper now reject non-decimal / unsafe values, and I did not find a new critical blocker.
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
Maintainer verification — local real-binary + tmux E2E ✅Built this PR from a clean worktree at the head ( Environment
1. Build / typecheck / focused tests — all green
2. Real tmux E2E — the actual
|
QWEN_SERVE_MCP_CLIENT_BUDGET |
breadcrumb | clientBudget | budgetMode | |
|---|---|---|---|---|
16 |
– | 16 |
warn |
✅ accepted |
100 |
– | 100 |
warn |
✅ accepted |
" 42 " |
– | 42 |
warn |
✅ accepted (trim) |
0x10 |
absent | off |
✅ rejected (was silently 16) |
|
1e2 |
absent | off |
✅ rejected (was 100) |
|
1.0 |
absent | off |
✅ rejected (was 1) |
|
0 |
absent | off |
✅ rejected (> 0) |
|
-5 |
absent | off |
✅ rejected | |
| (unset) | – | absent | off |
✅ no budget |
0x10 + MODE=enforce |
absent | off |
✅ rejected; enforce safely downgraded to off (no valid budget → never silently enforces) |
The three headline values (0x10/1e2/1.0) now produce the operator breadcrumb qwen serve: ignoring invalid QWEN_SERVE_MCP_CLIENT_BUDGET=... and resolve to no budget, instead of silently becoming 16/100/1.
3. Mutation A/B — the new tests genuinely catch the bug
Reverted only the parse condition back to the old loose Number() + isInteger in both files and re-ran the focused tests (then restored):
- core:
readBudgetFromEnv rejects non-decimal budget values→ FAILS withexpected 16 to be undefined(i.e.0x10→16 slips through the old parse). - cli: the
0x10/1e2/1.0/0b101rejection cases → FAIL (expected {…} to be undefined), while5 abc/abc/-5/0/" "still pass (the oldNumber()already rejected those asNaN/≤0).
This precisely isolates what the strict /^\d+$/ + isSafeInteger adds: rejecting hex / scientific / float / binary literals that Number() silently coerces. The tests are non-vacuous.
4. Completeness — no third parser missed
A repo-wide grep confirms exactly two functions parse the env string, and the PR fixes both:
packages/core/src/tools/mcp-client-manager.ts→readBudgetFromEnv()packages/cli/src/acp-integration/acpAgent.ts→createWorkspaceMcpBudget()
run-qwen-serve.ts only writes the var from a CLI-validated number (childEnvOverrides), and daemon-status.ts only copies an already-parsed number — neither re-parses the string. Number.isSafeInteger is also a small upgrade over the old Number.isInteger (rejects out-of-range magnitudes like 99999999999999999999).
Notes (minor, non-blocking)
- The
createWorkspaceMcpBudgetpath now also emits the stderr breadcrumb for invalid values (previously silent) — a welcome operator-visibility improvement that brings it in line with the manager. Slightly more than "pure parsing," but a net positive. - The parse logic is duplicated across
coreandcli, kept in sync by comments. Pre-existing pattern (not introduced here); a shared exported helper would remove the future-drift risk, but that's an optional follow-up.
Verdict
Correct, complete, well-tested, no regressions. The real binary rejects exactly the values the PR targets and accepts valid decimals. LGTM. 👍
🇨🇳 中文版（完整对应）
维护者验证 —— 本地真实二进制 + tmux 端到端 ✅
在 PR head(7ce2a8864)的干净 worktree 上构建本 PR,跑了 focused 测试,在 tmux 中驱动真实 qwen --acp 二进制跑了一组「非法/合法 budget」矩阵,并做了变异 A/B 证明新测试确实能抓到这个 bug。修复正确、完整、无回归。
环境
- macOS(darwin),Node v22.22.2,npm 10.9.7
- worktree 位于 PR head
7ce2a8864,全新npm ci+npm run build
1. 构建 / 类型检查 / focused 测试 —— 全绿
| 检查项 | 结果 |
|---|---|
npm run build(根) |
✅ exit 0(仅 vscode-ide-companion 既有 lint warning,0 error) |
npm run typecheck(根) |
✅ exit 0 |
core:mcp-client-manager.test.ts -t readBudgetFromEnv |
✅ 5 passed |
cli:acpAgent.test.ts -t createWorkspaceMcpBudget |
✅ 12 passed |
2. 真实 tmux 端到端 —— 真正会触发 bug 的 qwen --acp 路径
daemon(qwen serve)在 spawn child 时转发的是已校验的 number / 或直接 scrub 掉 env,所以松散 Number() bug 真正咬到的是直接 qwen --acp 路径 —— 此时 createWorkspaceMcpBudget()(以及 core 的 readBudgetFromEnv())直接读 env 字符串。我用 ndjson stdio 驱动真实构建出的二进制:initialize → qwen/status/workspace/mcp,读取暴露出的 clientBudget/budgetMode,并观察 stderr 的 breadcrumb。
QWEN_SERVE_MCP_CLIENT_BUDGET |
breadcrumb | clientBudget | budgetMode | |
|---|---|---|---|---|
16 |
– | 16 |
warn |
✅ 接受 |
100 |
– | 100 |
warn |
✅ 接受 |
" 42 " |
– | 42 |
warn |
✅ 接受(trim) |
0x10 |
无 | off |
✅ 拒绝(旧版静默变 16) |
|
1e2 |
无 | off |
✅ 拒绝(旧版 100) |
|
1.0 |
无 | off |
✅ 拒绝(旧版 1) |
|
0 |
无 | off |
✅ 拒绝(> 0) |
|
-5 |
无 | off |
✅ 拒绝 | |
| (未设置) | – | 无 | off |
✅ 无 budget |
0x10 + MODE=enforce |
无 | off |
✅ 拒绝;enforce 安全降级为 off(无有效 budget → 绝不静默执行) |
三个核心值(0x10/1e2/1.0)现在会打印 operator breadcrumb qwen serve: ignoring invalid QWEN_SERVE_MCP_CLIENT_BUDGET=... 并解析为无 budget,而不是静默变成 16/100/1。
3. 变异 A/B —— 新测试确实抓得到这个 bug
把两个文件的解析条件仅改回旧的松散 Number() + isInteger,复跑 focused 测试(随后还原):
- core:
readBudgetFromEnv rejects non-decimal budget values→ 失败,报expected 16 to be undefined(即0x10→16 从旧解析漏过)。 - cli:
0x10/1e2/1.0/0b101的拒绝用例 → 失败(expected {…} to be undefined),而5 abc/abc/-5/0/" "仍通过(旧Number()本就把它们判为NaN/≤0)。
这精确隔离出严格 /^\d+$/ + isSafeInteger 新增拦截的正是 Number() 会静默 coerce 的 hex / 科学计数 / 浮点 / 二进制字面量。测试非空过。
4. 完整性 —— 没有漏掉第三个 parser
全仓库 grep 确认恰好两个函数解析该 env 字符串,PR 两个都修了:
packages/core/src/tools/mcp-client-manager.ts→readBudgetFromEnv()packages/cli/src/acp-integration/acpAgent.ts→createWorkspaceMcpBudget()
run-qwen-serve.ts 只是把 CLI 校验过的 number 写入 env(childEnvOverrides),daemon-status.ts 只是复制已解析的 number —— 都不重新解析字符串。Number.isSafeInteger 相比旧的 Number.isInteger 也是个小升级(能拒绝 99999999999999999999 这类超范围量级)。
备注(次要,不阻塞)
createWorkspaceMcpBudget路径现在对非法值也会输出 stderr breadcrumb(以前是静默)—— 这是个不错的 operator 可见性改进,和 manager 对齐了。比「纯解析」略多一点,但是净收益。- 解析逻辑在
core和cli两处重复,靠注释保持同步。这是既有模式(非本 PR 引入);提取一个共享导出 helper 可消除未来漂移风险,属可选后续。
结论
正确、完整、测试充分、无回归。真实二进制恰好拒绝 PR 针对的那些值、接受合法十进制。LGTM。👍
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
[Suggestion] Stale JSDoc at mcp-client-manager.ts:271-274 — the doc comment says invalid budget values are "silently ignored (treated as unset)", but the function emits a stderr breadcrumb for invalid values. Since this PR modifies the parsing logic in this function, the doc should be updated to match reality:
* `QWEN_SERVE_MCP_CLIENT_BUDGET` — positive integer; non-numeric /
* zero / negative / NaN are rejected with a stderr warning and
* treated as unset.
(Not inline-commentable because the JSDoc lines are outside the diff range.)
Previous Critical (acpAgent.ts loose parsing) is resolved. No blockers remain.
— qwen3.7-max via Qwen Code /review
The doc still said invalid budget values are silently ignored, but the function now writes a stderr breadcrumb when it rejects one. Match the doc to the behavior.
|
Good catch — updated the JSDoc in f892053 so it says invalid values are rejected and a stderr breadcrumb is written, matching what the function now does. |
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Both parsing sites now use the identical strict pipeline: trim() → /^\d+$/ → Number.isSafeInteger() → > 0. The JSDoc is updated to match the stderr breadcrumb behavior. Tests pass.
— qwen3.7-max via Qwen Code /review
✅ Local verification — strict-decimal budget parsingI built this branch locally and verified it end-to-end with a real before/after harness driving the compiled output (not just the unit tests). Verdict: behaves exactly as described — safe to merge. What I droveBoth real parsing paths, against the built
Method
Before → after (real compiled output)① Silently-coerced values — the actual bug (now rejected on both paths):
② Already-invalid values — outcome unchanged, but the cli path now emits the operator-visibility breadcrumb (core already did):
③ Unchanged — no regression:
Tests
Notes
LGTM 👍 中文说明✅ 本地验证 —— 严格十进制预算解析我在本地构建了该分支，并用 真实的 before/after 测试脚本驱动编译产物（dist） 做了端到端验证（不仅仅依赖单元测试）。结论：行为与描述完全一致，可以安全合并。 验证了什么两条真实的解析路径，均针对构建后的
方法
Before → after（真实编译产物）① 被静默强转的取值 —— 真正的缺陷（修复后两条路径均拒绝）：
② 本就非法的取值 —— 结果不变，但 cli 路径现在会输出便于运维排查的 breadcrumb（core 之前已有）：
③ 不变 —— 无回归：
测试
备注
LGTM 👍 🤖 Verified locally with Claude Code (model: Opus 4.8). |
5 participants
What
readBudgetFromEnvparsedQWEN_SERVE_MCP_CLIENT_BUDGETwithNumber(rawBudget), soNumber("0x10")=16,Number("1e2")=100andNumber("1.0")=1all passedNumber.isIntegerand were silently accepted as a budget. This requires plain decimal digits (/^\d+$/), matching the other integer env vars (#5679, #5602, #5612), and keeps the existing stderr breadcrumb for invalid values.Why
A hex / scientific / float string silently became a budget the operator never intended (e.g.
QWEN_SERVE_MCP_CLIENT_BUDGET=0x10→ 16) instead of being rejected like every other malformed value. This finishes the strict-decimal integer-env parsing the codebase already adopted elsewhere.Reviewer Test Plan
npx vitest run packages/core/src/tools/mcp-client-manager.test.ts -t readBudgetFromEnvThe added test asserts
0x10/1e2/1.0are rejected (no budget resolved) while16is still accepted. It fails on the old looseNumber()parse and passes with this change.Risk
Low. Pure parsing change in one function; valid decimal budgets are unaffected and the existing stderr breadcrumb still fires for rejected values.