Skip to content

fix(core): parse QWEN_SERVE_MCP_CLIENT_BUDGET strictly as a decimal integer #5752

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wenshao merged 3 commits into from
Jun 24, 2026
Merged

fix(core): parse QWEN_SERVE_MCP_CLIENT_BUDGET strictly as a decimal integer #5752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
162c38d
fix(core): parse QWEN_SERVE_MCP_CLIENT_BUDGET strictly as a decimal i…
he-yufeng Jun 23, 2026
7ce2a88
fix(acp): apply strict budget-env parsing to createWorkspaceMcpBudget
he-yufeng Jun 23, 2026
f892053
docs: sync readBudgetFromEnv JSDoc with the stderr breadcrumb
he-yufeng Jun 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions packages/cli/src/acp-integration/acpAgent.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,6 +513,7 @@ import {
normalizeCoreSettingValue,
extractFilesFromTarGz,
fetchAllowedGitHub,
createWorkspaceMcpBudget,
} from './acpAgent.js';
import { gzipSync } from 'node:zlib';
import type { Config } from '@qwen-code/qwen-code-core';
Expand Down Expand Up @@ -6713,3 +6714,40 @@ describe('sessionLanguage multi-session propagation', () => {
await agentPromise;
});
});

describe('createWorkspaceMcpBudget — env parsing', () => {
const KEY = 'QWEN_SERVE_MCP_CLIENT_BUDGET';
const MODE = 'QWEN_SERVE_MCP_BUDGET_MODE';
const onEvent = vi.fn();

afterEach(() => {
delete process.env[KEY];
delete process.env[MODE];
vi.clearAllMocks();
});

it('accepts a plain positive decimal integer', () => {

@qwen-code-ci-bot qwen-code-ci-bot Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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();
});

process.env[KEY] = '100';

@qwen-code-ci-bot qwen-code-ci-bot Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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' }),
);

expect(createWorkspaceMcpBudget(onEvent)).toBeDefined();
});

it('accepts a trimmed decimal integer', () => {
process.env[KEY] = ' 42 ';
expect(createWorkspaceMcpBudget(onEvent)).toBeDefined();
});

// Mirrors McpClientManager.readBudgetFromEnv: a loose Number() would coerce
// these (0x10=16, 1e2=100, 1.0=1) and silently set a budget. The strict
// /^\d+$/ + isSafeInteger parse must reject them.
it.each(['0x10', '1e2', '1.0', '0b101', '5 abc', 'abc', '-5', '0', ' '])(
'rejects non-decimal-integer value %j',
(raw) => {
process.env[KEY] = raw;
expect(createWorkspaceMcpBudget(onEvent)).toBeUndefined();
},
);

it('returns undefined when the budget env var is unset', () => {
expect(createWorkspaceMcpBudget(onEvent)).toBeUndefined();
});
});
39 changes: 21 additions & 18 deletions packages/cli/src/acp-integration/acpAgent.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2489,34 +2489,37 @@ function parsePoolDrainMs(envValue: string | undefined): number {
* invokes `tryReserve`/`release`; this helper produces the controller
* and wires the event callback.
*/
function createWorkspaceMcpBudget(
export function createWorkspaceMcpBudget(
onEvent: (event: McpBudgetEvent) => void,
): WorkspaceMcpBudget | undefined {
const rawBudget = process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'];
const rawMode = process.env['QWEN_SERVE_MCP_BUDGET_MODE'];
// 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

@qwen-code-ci-bot qwen-code-ci-bot Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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:

  1. readBudgetFromEnv emits a stderr breadcrumb when QWEN_SERVE_MCP_BUDGET_MODE is set to an unrecognized value (e.g. ENFORCE); createWorkspaceMcpBudget silently falls through to the default.
  2. readBudgetFromEnv emits a stderr breadcrumb when mode is enforce/warn but no budget is set (downgrading to off); createWorkspaceMcpBudget returns undefined silently.

A future maintainer reading "exactly" may skip checking readBudgetFromEnv for features to replicate here. Consider scoping the claim:

Suggested change
// decimal digits set a budget. A loose `Number(...)` would silently accept
// Match `McpClientManager.readBudgetFromEnv`'s budget-value parsing:

// `0x10`=16, `1e2`=100, and `1.0`=1 (all pass `isInteger`); the strict
// `/^\d+$/` + `isSafeInteger` check rejects them so the pool and the manager
// honor the same env values.
const budget =
rawBudget !== undefined && rawBudget !== '' ? Number(rawBudget) : undefined;
let budget: number | undefined;
if (rawBudget !== undefined && rawBudget !== '') {
const trimmed = rawBudget.trim();
const parsed = Number(trimmed);
if (/^\d+$/.test(trimmed) && Number.isSafeInteger(parsed) && parsed > 0) {
budget = parsed;
} else {
process.stderr.write(
`qwen serve: ignoring invalid QWEN_SERVE_MCP_CLIENT_BUDGET=` +
`'${rawBudget}' (expected positive integer); ` +
`MCP budget enforcement disabled for this child.\n`,
);
}
}
const mode: McpBudgetMode = (() => {
if (rawMode === 'enforce' || rawMode === 'warn' || rawMode === 'off') {
return rawMode;
}
return budget !== undefined &&
Number.isFinite(budget) &&
Number.isInteger(budget) &&
budget > 0
? 'warn'
: 'off';
return budget !== undefined ? 'warn' : 'off';
})();
if (
mode === 'off' ||
budget === undefined ||
!Number.isFinite(budget) ||
!Number.isInteger(budget) ||
budget <= 0
) {
if (mode === 'off' || budget === undefined) {
return undefined;
}
return new WorkspaceMcpBudget({
Expand Down
19 changes: 19 additions & 0 deletions packages/core/src/tools/mcp-client-manager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2627,6 +2627,25 @@ describe('McpClientManager — PR 14 guardrails', () => {
}
});

it('readBudgetFromEnv rejects non-decimal budget values (hex / scientific / float)', async () => {
const writeSpy = vi.spyOn(process.stderr, 'write').mockReturnValue(true);

@doudouOUC doudouOUC Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

try {
for (const bad of ['0x10', '1e2', '1.0']) {
process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'] = bad;
const manager = mkManager({ config: configWithServers({}) });

@doudouOUC doudouOUC Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

// Pre-fix Number('0x10')=16 / Number('1e2')=100 slipped through as a budget.
expect(manager.getMcpClientBudget()).toBeUndefined();
}
// 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);

@doudouOUC doudouOUC Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

} finally {
writeSpy.mockRestore();
delete process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'];
}
});

it('readResource rejects existing-but-now-disabled servers (wenshao R7 #5 line 1342)', async () => {
// Pre-fix: a server connected pre-disable and then operator-
// disabled mid-session via settings reload would still serve
Expand Down
11 changes: 8 additions & 3 deletions packages/core/src/tools/mcp-client-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,7 +269,8 @@ export function mcpTransportOf(config: MCPServerConfig): McpTransportKind {
* behavior, no enforcement.
*
* `QWEN_SERVE_MCP_CLIENT_BUDGET` — positive integer; non-numeric /
* zero / negative / NaN are silently ignored (treated as unset).
* zero / negative / NaN are rejected (treated as unset) and a
* stderr breadcrumb is written so the misconfiguration is visible.
* `QWEN_SERVE_MCP_BUDGET_MODE` — `enforce|warn|off`. Defaults to
* `warn` when a budget is set, `off` otherwise.
*/
Expand All @@ -278,8 +279,12 @@ function readBudgetFromEnv(): McpBudgetConfig {
const rawMode = process.env['QWEN_SERVE_MCP_BUDGET_MODE'];
let clientBudget: number | undefined;
if (rawBudget !== undefined && rawBudget !== '') {
const parsed = Number(rawBudget);
if (Number.isFinite(parsed) && Number.isInteger(parsed) && parsed > 0) {
// Parse strictly as a decimal integer: Number('0x10')=16, Number('1e2')=100
// 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);

@doudouOUC doudouOUC Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

if (/^\d+$/.test(trimmed) && Number.isSafeInteger(parsed) && parsed > 0) {
clientBudget = parsed;
} else {
// operator typos
Expand Down
Loading