vrr/qwen-code
2
0
Fork 0
mirror of https://github.com/QwenLM/qwen-code.git synced 2026-07-10 01:29:17 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 12

fixup(serve): address PR 14 review (#4247) findings 1-7

Browse source 
Addresses Codex + Copilot review feedback on #4247. Seven functional and forward-compat fixes; (8) `tcp` transport mapper vs createTransport deferred pending @wenshao direction (separate core/protocol decision).

1. **Single-server rediscovery bypass** — add `tryReserveSlot` at the top of `discoverMcpToolsForServerInternal`. Pre-fix a server refused at startup could be brought online later via `/mcp reconnect <name>` and exceed the cap in enforce mode.
2. **Empty `budgets[]` when mode=off** — early `return []` in `buildBudgetCells` when mode is `off`. Protocol docs / SDK types promise empty array; pre-fix emitted a synthetic noisy cell.
3. **runQwenServe validation + env leakage** — mirror CLI budget validation in `runQwenServe` (the embedded entry point); explicitly delete `QWEN_SERVE_MCP_*` env vars when options are undefined so multiple daemons in one process don't leak prior budget config to subsequent ACP children.
4. **Disabled-vs-refused precedence + stale refusal log** — config-disable wins over budget refusal in the per-server cell; `removeServer` + `disconnectServer` drop the entry from `lastRefusedServerNames` so operator action immediately clears the budget tag.
5. **Incremental remove-before-reserve ordering** — process config-removed servers FIRST in `discoverAllMcpToolsIncremental` so freed slots are visible to subsequent `tryReserveSlot` calls. Pre-fix scenario {a,b}→{a,c} with budget=2 wasted a slot.
6. **`scope` forward-compat type widening** — `'workspace' | (string & {})` on both `ServeMcpBudgetStatusCell` and `DaemonMcpBudgetStatusCell` so SDK consumers don't break when PR 23 adds `scope: 'pool'` per the documented no-schema-bump contract.
7. **Test comment alignment** — fix "With budget=1" comment to match `clientBudget: 2` code.

Plus 4 new core regression tests covering #1/#2/#4/#5, and 4 new serve tests covering #3 (boot rejection + env cleanup). 237/237 pass across the affected files (36 core mcp-client-manager + 50 acpAgent + 151 serve).
This commit is contained in:
doudouOUC 2026-05-17 23:31:14 +08:00
parent c4218ca7d1
commit 597f011e66
7 changed files with 342 additions and 22 deletions
Download patch file Download diff file Expand all files Collapse all files

24
packages/cli/src/acp-integration/acpAgent.ts
View file

@ -691,13 +691,23 @@ class QwenAgent implements Agent {
const disabled = config.isMcpServerDisabled(name);
const rawStatus = getMCPServerStatus(name);
const refusedByBudget = refusedSet.has(name);
// PR 14 fix (review #4247): config-disable takes precedence
// over budget-refusal. `lastRefusedServerNames` is a
// per-discovery-pass snapshot; if an operator runs
// `/mcp disable <name>` against a server that was refused
// last pass, the entry stays in the refused list until the
// next discovery pass clears it (`McpClientManager.removeServer`
// now drops the entry too — see sibling fix). Either way,
// a `disabled` cell should NEVER show `budget_exhausted` —
// the operator's deliberate disable wins.
const effectivelyRefused = refusedByBudget && !disabled;
const out: ServeWorkspaceMcpServerStatus = {
kind: 'mcp_server',
// Refused-by-budget shadows the raw status: the rawStatus
// is `DISCONNECTED` (we never tried to connect), but the
// operator-facing severity is `error` with an explanatory
// errorKind rather than the generic disconnected `error`.
status: refusedByBudget
status: effectivelyRefused
? 'error'
: this.mcpCellStatus(rawStatus, disabled),
name,
@ -705,7 +715,7 @@ class QwenAgent implements Agent {
transport: this.mcpTransport(server),
disabled,
};
if (refusedByBudget) {
if (effectivelyRefused) {
out.errorKind = 'budget_exhausted';
out.disabledReason = 'budget';
out.hint =
@ -772,6 +782,16 @@ class QwenAgent implements Agent {
mode: ServeMcpBudgetMode,
refusedCount: number,
): ServeMcpBudgetStatusCell[] {
// PR 14 fix (review #4247): when no `--mcp-client-budget` is
// configured the manager resolves to `mode: 'off'`. The protocol
// docs and SDK type comments promise `budgets: []` for that case;
// a synthetic `mcp_budget` cell carrying nothing actionable was
// (a) protocol-noncompliant, (b) clutter — clients iterating
// `budgets[]` to render rows would draw an "ok" budget row for
// uncapped workspaces. Always return empty so the top-level
// `budgetMode: 'off'` field is the sole signal that guardrails
// are inactive.
if (mode === 'off') return [];
let status: ServeStatus = 'ok';
let errorKind: string | undefined;
let hint: string | undefined;

31
packages/cli/src/serve/runQwenServe.ts
View file

@ -185,11 +185,42 @@ export async function runQwenServe(
// `process.env` via `{...process.env}` at spawn time. Standalone
// `qwen` invocations (no daemon) leave the env untouched, so
// direct CLI usage retains the historical no-enforcement default.
//
// PR 14 fix (review #4247): `runQwenServe` is exported and other
// validations (`requireAuth` no-token, `maxConnections` NaN,
// `--workspace` checks) live here, so embedded callers expect
// boot-time rejection of invalid inputs. The yargs CLI handler
// duplicates these checks for fast-fail UX, but `runQwenServe` is
// the source of truth. Also explicitly DELETE (vs leaving stale)
// the env vars when the caller omits the option, so an embedded
// process that starts multiple daemons in sequence doesn't leak
// a prior budget/mode into the next ACP child.
if (opts.mcpClientBudget !== undefined) {
if (
!Number.isFinite(opts.mcpClientBudget) ||
!Number.isInteger(opts.mcpClientBudget) ||
opts.mcpClientBudget <= 0
) {
throw new TypeError(
`Invalid mcpClientBudget: ${opts.mcpClientBudget}. Must be a positive integer.`,
);
}
}
if (opts.mcpBudgetMode === 'enforce' && opts.mcpClientBudget === undefined) {
throw new Error(
'mcpBudgetMode="enforce" requires a positive mcpClientBudget. ' +
'Pass mcpClientBudget=N, or set mcpBudgetMode to "warn" or "off".',
);
}
if (opts.mcpClientBudget !== undefined) {
process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'] = String(opts.mcpClientBudget);
} else {
delete process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'];
}
if (opts.mcpBudgetMode !== undefined) {
process.env['QWEN_SERVE_MCP_BUDGET_MODE'] = opts.mcpBudgetMode;
} else {
delete process.env['QWEN_SERVE_MCP_BUDGET_MODE'];
}
const bridge =

69
packages/cli/src/serve/server.test.ts
View file

@ -2661,6 +2661,75 @@ describe('runQwenServe', () => {
).rejects.toThrow(/--require-auth/);
});
// PR 14 fix (review #4247): runQwenServe is the documented embedded
// entry point, so budget validation must live here, not just in the
// yargs CLI handler. Embedded callers (other tools wrapping the
// daemon, deps.bridge test injection) silently produced an uncapped
// child pre-fix despite requesting enforce.
it('rejects non-positive mcpClientBudget (#4175 PR 14)', async () => {
await expect(
runQwenServe({
hostname: '127.0.0.1',
port: 0,
mode: 'http-bridge',
mcpClientBudget: 0,
}),
).rejects.toThrow(/mcpClientBudget/);
await expect(
runQwenServe({
hostname: '127.0.0.1',
port: 0,
mode: 'http-bridge',
mcpClientBudget: -5,
}),
).rejects.toThrow(/mcpClientBudget/);
});
it('rejects mcpBudgetMode=enforce without a budget (#4175 PR 14)', async () => {
await expect(
runQwenServe({
hostname: '127.0.0.1',
port: 0,
mode: 'http-bridge',
mcpBudgetMode: 'enforce',
}),
).rejects.toThrow(/enforce.*requires.*mcpClientBudget/);
});
it('clears mcp env vars when caller omits the options (no leakage across daemons)', async () => {
process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'] = '99';
process.env['QWEN_SERVE_MCP_BUDGET_MODE'] = 'enforce';
try {
handle = await runQwenServe({
hostname: '127.0.0.1',
port: 0,
mode: 'http-bridge',
// No mcpClientBudget / mcpBudgetMode — must wipe stale env
// from the previous daemon in the same process.
});
expect(process.env['QWEN_SERVE_MCP_CLIENT_BUDGET']).toBeUndefined();
expect(process.env['QWEN_SERVE_MCP_BUDGET_MODE']).toBeUndefined();
} finally {
delete process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'];
delete process.env['QWEN_SERVE_MCP_BUDGET_MODE'];
}
});
it('writes mcp env vars when caller provides the options', async () => {
handle = await runQwenServe({
hostname: '127.0.0.1',
port: 0,
mode: 'http-bridge',
mcpClientBudget: 10,
mcpBudgetMode: 'warn',
});
expect(process.env['QWEN_SERVE_MCP_CLIENT_BUDGET']).toBe('10');
expect(process.env['QWEN_SERVE_MCP_BUDGET_MODE']).toBe('warn');
// Cleanup so the next test starts clean.
delete process.env['QWEN_SERVE_MCP_CLIENT_BUDGET'];
delete process.env['QWEN_SERVE_MCP_BUDGET_MODE'];
});
it('starts with --require-auth + token on loopback', async () => {
handle = await runQwenServe({
hostname: '127.0.0.1',

14
packages/cli/src/serve/status.ts
View file

@ -83,13 +83,21 @@ export type ServeMcpBudgetMode = 'enforce' | 'warn' | 'off';
*/
export interface ServeMcpBudgetStatusCell extends ServeStatusCell {
kind: 'mcp_budget';
/** Identifies which accounting scope this cell describes. */
scope: 'workspace';
/**
* Identifies which accounting scope this cell describes. Today only
* `'workspace'` is emitted; future PRs (e.g. Wave 5 PR 23 shared
* pool) will add `'pool'`. The `string & {}` widening keeps IDE
* autocomplete + literal narrowing for known scopes while allowing
* unknown scopes through without a compile-time break — the
* protocol contract is "consumers MUST tolerate additional scope
* values, drop don't fail." See `qwen-serve-protocol.md` forward-compat note.
*/
scope: 'workspace' | (string & {});
/** Live (CONNECTED) MCP client count at snapshot time. */
liveCount: number;
/** Configured cap (positive integer). Absent only when mode is `off`. */
budget?: number;
/** Active enforcement mode. `off` cells SHOULD still be reported with status `ok`. */
/** Active enforcement mode. `off` mode produces no cell — `budgets: []`. */
mode: ServeMcpBudgetMode;
/** Servers refused during the most recent discovery pass. */
refusedCount: number;

141
packages/core/src/tools/mcp-client-manager.test.ts
View file

@ -1239,8 +1239,9 @@ describe('McpClientManager — PR 14 guardrails', () => {
created.push(name);
return makeConnectedMcpClientMock() as unknown as McpClient;
});
// `b` is disabled — must not even attempt to reserve. With budget=1,
// `a` and `c` should both succeed (b is invisible to the gate).
// `b` is disabled — must not even attempt to reserve. With budget=2,
// `a` and `c` should both succeed (b is invisible to the gate, so it
// doesn't consume a slot; the cap is enough for the remaining two).
const config = configWithServers(
{
a: { command: 'node' },
@ -1268,4 +1269,140 @@ describe('McpClientManager — PR 14 guardrails', () => {
]);
expect(manager.getMcpClientAccounting().refusedServerNames).toEqual([]);
});
// PR 14 fix (review #4247): regression tests for the four bypass /
// ordering / staleness bugs the Codex + Copilot reviews caught.
it('single-server rediscovery respects the budget gate (review #1)', async () => {
const created: string[] = [];
vi.mocked(McpClient).mockImplementation((name: string) => {
created.push(name);
return makeConnectedMcpClientMock() as unknown as McpClient;
});
const config = configWithServers({
a: { command: 'node' },
b: { command: 'node' },
});
const manager = new McpClientManager(
config,
{} as ToolRegistry,
undefined,
undefined,
undefined,
{ clientBudget: 1, budgetMode: 'enforce' },
);
await manager.discoverAllMcpTools(config);
// `b` was refused at startup. A manual `/mcp reconnect b` (which goes
// through `discoverMcpToolsForServer` → `...Internal`) would have
// pre-fix bypassed the gate and exceeded the cap. Now it must stay
// refused.
expect(created).toEqual(['a']);
expect(manager.getMcpClientAccounting().refusedServerNames).toEqual(['b']);
await manager.discoverMcpToolsForServer('b', config);
expect(created).toEqual(['a']); // no new McpClient created
expect(manager.getMcpClientAccounting().reservedSlots).toEqual(['a']);
expect(manager.getMcpClientAccounting().refusedServerNames).toEqual(['b']);
});
it('disconnectServer-then-disable drops refusal tag (review #4)', async () => {
vi.mocked(McpClient).mockImplementation(
() => makeConnectedMcpClientMock() as unknown as McpClient,
);
const config = configWithServers({
a: { command: 'node' },
b: { command: 'node' },
});
const manager = new McpClientManager(
config,
{} as ToolRegistry,
undefined,
undefined,
undefined,
{ clientBudget: 1, budgetMode: 'enforce' },
);
await manager.discoverAllMcpTools(config);
expect(manager.getMcpClientAccounting().refusedServerNames).toEqual(['b']);
// Operator action: explicit disconnect of `b` should drop it from
// the refusal log so a snapshot doesn't keep tagging the now-
// operator-disabled server with `budget_exhausted`.
await manager.disconnectServer('b');
expect(manager.getMcpClientAccounting().refusedServerNames).toEqual([]);
});
it('incremental discovery frees removed slots BEFORE reserving new ones (review #5)', async () => {
let inflight = 0;
vi.mocked(McpClient).mockImplementation(
() => makeConnectedMcpClientMock() as unknown as McpClient,
);
inflight = 0;
const mcpServers: Record<string, { command: string }> = {
a: { command: 'node' },
b: { command: 'node' },
};
const config = {
isTrustedFolder: () => true,
getMcpServers: () => mcpServers,
getMcpServerCommand: () => undefined,
getPromptRegistry: () => ({}) as PromptRegistry,
getWorkspaceContext: () => ({}) as WorkspaceContext,
getDebugMode: () => false,
isMcpServerDisabled: () => false,
} as unknown as Config;
const manager = new McpClientManager(
config,
{
removeMcpToolsByServer: () => undefined,
} as unknown as ToolRegistry,
undefined,
undefined,
undefined,
{ clientBudget: 2, budgetMode: 'enforce' },
);
await manager.discoverAllMcpToolsIncremental(config);
expect(manager.getMcpClientAccounting().reservedSlots.sort()).toEqual([
'a',
'b',
]);
// Swap b → c (still budget=2). Pre-fix order: `c` refused because
// `b`'s slot was only freed after the new-server loop. Post-fix:
// `b` removed first → reservedSlots={a} → `c` reserved.
delete mcpServers['b'];
mcpServers['c'] = { command: 'node' };
await manager.discoverAllMcpToolsIncremental(config);
expect(manager.getMcpClientAccounting().reservedSlots.sort()).toEqual([
'a',
'c',
]);
expect(manager.getMcpClientAccounting().refusedServerNames).toEqual([]);
void inflight;
});
it('buildBudgetCells deferred to acpAgent — manager off-mode returns no budget bookkeeping (review #2)', async () => {
// Sibling check: when `mode === 'off'` the manager doesn't reserve
// anything and the snapshot has empty `reservedSlots` + zero
// `refusedServerNames`. The empty-`budgets[]` assertion lives in
// the serve route test (`server.test.ts`) because the cell is
// built by `acpAgent.buildBudgetCells`. This test just pins the
// manager-side invariant: off-mode is pure observability.
vi.mocked(McpClient).mockImplementation(
() => makeConnectedMcpClientMock() as unknown as McpClient,
);
const config = configWithServers({
a: { command: 'node' },
b: { command: 'node' },
});
const manager = new McpClientManager(
config,
{} as ToolRegistry,
undefined,
undefined,
undefined,
{ budgetMode: 'off' },
);
await manager.discoverAllMcpTools(config);
const accounting = manager.getMcpClientAccounting();
expect(accounting.total).toBe(2);
expect(accounting.reservedSlots).toEqual([]);
expect(accounting.refusedServerNames).toEqual([]);
});
});

75
packages/core/src/tools/mcp-client-manager.ts
View file

@ -447,6 +447,28 @@ export class McpClientManager {
return;
}
// PR 14 fix (review #4247): single-server rediscovery (reachable from
// `/mcp reconnect <name>` and `ToolRegistry.discoverToolsForServer`)
// previously bypassed the budget gate, so a server refused at startup
// could be brought online later under `enforce` mode and exceed the
// cap. True reconnect against a held slot returns `'already_held'`
// and falls through unchanged; only a fresh attempt against a server
// without a reservation can be refused. Best-effort semantics — log
// the refusal and return without creating an `McpClient`; the caller
// observes the absence via `getStatus()` like any other discovery
// failure.
const reservation = this.tryReserveSlot(serverName);
if (reservation === 'refused') {
if (!this.lastRefusedServerNames.includes(serverName)) {
this.lastRefusedServerNames.push(serverName);
}
process.stderr.write(
`qwen serve: MCP server '${serverName}' refused (budget exhausted, ` +
`budget=${this.clientBudget}, mode=enforce)\n`,
);
return;
}
this.stopHealthCheck(serverName);
// Ensure we don't leak an existing connection for this server.
@ -552,15 +574,23 @@ export class McpClientManager {
this.consecutiveFailures.delete(serverName);
this.isReconnecting.delete(serverName);
this.serverDiscoveryPromises.delete(serverName);
// PR 14: explicit operator-driven disconnect releases the
// budget slot. The internal reconnect path
// (`discoverMcpToolsForServerInternal`) calls
// `existingClient.disconnect()` directly, NOT this public
// method, so reconnect doesn't release.
this.reservedSlots.delete(serverName);
this.eventEmitter?.emit('mcp-client-update', this.clients);
}
}
// PR 14: explicit operator-driven disconnect releases the budget
// slot AND drops the entry from the per-pass refusal log. Outside
// the `if (client)` guard because a budget-refused server has NO
// `McpClient` instance — but operator intent ("stop tracking this
// server") still demands the records be cleared so a subsequent
// snapshot doesn't keep tagging it as `budget_exhausted`. The
// internal reconnect path (`discoverMcpToolsForServerInternal`)
// calls `existingClient.disconnect()` directly, NOT this public
// method, so reconnect still doesn't release the slot.
this.reservedSlots.delete(serverName);
const refusedIdx = this.lastRefusedServerNames.indexOf(serverName);
if (refusedIdx >= 0) {
this.lastRefusedServerNames.splice(refusedIdx, 1);
}
}
getDiscoveryState(): MCPDiscoveryState {
@ -755,6 +785,22 @@ export class McpClientManager {
const currentServerNames = new Set(this.clients.keys());
const newServerNames = new Set(Object.keys(servers));
// PR 14 fix (review #4247): process removals BEFORE the new-server
// reservation pass so freed slots are visible to `tryReserveSlot`.
// Scenario: budget=2, currently `{a, b}` reserved, new config
// `{a, c}`. Pre-fix order refused `c` because `b`'s slot was only
// freed after the new-server loop. Now `b` is removed first →
// reservedSlots={a} → `c` reservation succeeds. Disabled-mid-session
// removals stay inline (below) because they also release slots
// via `removeServer`'s `reservedSlots.delete` — same call, just
// reached from a different branch.
for (const name of currentServerNames) {
if (!newServerNames.has(name)) {
// Server was removed from configuration
await this.removeServer(name);
}
}
// Check for new servers or configuration changes
for (const [name] of Object.entries(servers)) {
// Mirror `discoverAllMcpTools` (line ~102): users who explicitly
@ -801,14 +847,6 @@ export class McpClientManager {
// the old and new config, which is not implemented here
}
// Find removed servers
for (const name of currentServerNames) {
if (!newServerNames.has(name)) {
// Server was removed from configuration
await this.removeServer(name);
}
}
// Update only the servers that need it. Each per-server discover is
// wrapped in a discovery-only timeout (stdio default 30s, remote 5s,
// per-server override via `discoveryTimeoutMs`). Tool-call timeout is
@ -1043,6 +1081,15 @@ export class McpClientManager {
// be running", so it must not block a different server from taking
// its place on the next discovery pass.
this.reservedSlots.delete(serverName);
// PR 14 fix (review #4247): also drop the entry from the per-pass
// refusal log so a snapshot taken between discoveries doesn't
// stale-tag the (now-disabled or now-removed) server as
// `disabledReason: 'budget'`. Operator action wins over the
// last-pass startup refusal record.
const refusedIdx = this.lastRefusedServerNames.indexOf(serverName);
if (refusedIdx >= 0) {
this.lastRefusedServerNames.splice(refusedIdx, 1);
}
// Remove tools for this server from registry
this.toolRegistry.removeMcpToolsByServer(serverName);

10
packages/sdk-typescript/src/daemon/types.ts
View file

@ -233,7 +233,15 @@ export type DaemonMcpBudgetMode = 'enforce' | 'warn' | 'off';
*/
export interface DaemonMcpBudgetStatusCell extends DaemonStatusCell {
kind: 'mcp_budget';
scope: 'workspace';
/**
* Today only `'workspace'` is emitted; future PRs (e.g. Wave 5
* PR 23 shared MCP pool) will add `'pool'` without a schema bump.
* The `string & {}` widening keeps IDE autocomplete + literal
* narrowing for known scopes while allowing unknown scopes through
* — the protocol contract is "consumers MUST tolerate additional
* scope values, drop don't fail." See `qwen-serve-protocol.md`.
*/
scope: 'workspace' | (string & {});
liveCount: number;
/** Configured cap. Absent when mode is `off`. */
budget?: number;