Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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', () => {
process.env[KEY] = '100';
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
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
// `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
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);

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

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);

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
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);

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