Skip to content
Open
Show file tree
Hide file tree
Changes from 4 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
16 changes: 11 additions & 5 deletions packages/cli/src/acp-integration/acpAgent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1777,7 +1777,12 @@ class QwenAgent implements Agent {
resume?: boolean,
): Promise<Config> {
this.settings = loadSettings(cwd);
const mergedMcpServers = { ...this.settings.merged.mcpServers };
// ACP/IDE-injected servers are session-level: they must outrank a project
// `.mcp.json` and stay un-gated. Collect them separately and pass them as
// `sessionMcpServers` (top precedence tier) rather than merging into
// `settings.mcpServers`, where `assembleMcpServers` would demote them below
// `.mcp.json` (#4615).
const sessionMcpServers: Record<string, MCPServerConfig> = {};

for (const server of mcpServers) {
const stdioServer = toStdioServer(server);
Expand All @@ -1786,7 +1791,7 @@ class QwenAgent implements Agent {
for (const { name: envName, value } of stdioServer.env) {
env[envName] = value;
}
mergedMcpServers[stdioServer.name] = new MCPServerConfig(
sessionMcpServers[stdioServer.name] = new MCPServerConfig(
stdioServer.command,
stdioServer.args,
env,
Expand All @@ -1801,7 +1806,7 @@ class QwenAgent implements Agent {
for (const { name: headerName, value } of sseServer.headers) {
headers[headerName] = value;
}
mergedMcpServers[sseServer.name] = new MCPServerConfig(
sessionMcpServers[sseServer.name] = new MCPServerConfig(
undefined,
undefined,
undefined,
Expand All @@ -1819,7 +1824,7 @@ class QwenAgent implements Agent {
for (const { name: headerName, value } of httpServer.headers) {
headers[headerName] = value;
}
mergedMcpServers[httpServer.name] = new MCPServerConfig(
sessionMcpServers[httpServer.name] = new MCPServerConfig(
undefined,
undefined,
undefined,
Expand All @@ -1832,7 +1837,7 @@ class QwenAgent implements Agent {
}
}

const settings = { ...this.settings.merged, mcpServers: mergedMcpServers };
const settings = this.settings.merged;
const argvForSession = {
...this.argv,
...(resume ? { resume: sessionId } : { sessionId }),
Expand All @@ -1849,6 +1854,7 @@ class QwenAgent implements Agent {
userHooks: this.settings.getUserHooks(),
projectHooks: this.settings.getProjectHooks(),
},
sessionMcpServers,
);
// PR 14b fix #2 (codex review round 1): register the MCP guardrail
// budget-event callback BEFORE `config.initialize()`. Pre-fix the
Expand Down
16 changes: 13 additions & 3 deletions packages/cli/src/commands/mcp.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,11 @@ describe('mcp command', () => {
it('should have exactly one option (help flag)', () => {
// Test to ensure that the global 'gemini' flags are not added to the mcp command
const yargsInstance = yargs();
const builtYargs = mcpCommand.builder(yargsInstance);
const builder = mcpCommand.builder;
if (typeof builder !== 'function') {
throw new Error('mcp command builder must be a function');
}
const builtYargs = builder(yargsInstance);
const options = builtYargs.getOptions();
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.

[Critical] TypeScript error: Property 'getOptions' does not exist on type 'Argv<{}> | PromiseLike<Argv<{}>>'. The builder function (typed as CommandModule['builder']) can return Argv | PromiseLike<Argv>. The runtime typeof builder !== 'function' guard narrows it to a function, but the return type is still the union.

Suggested change
const options = builtYargs.getOptions();
const builtYargs = await builder(yargsInstance);
const options = builtYargs.getOptions();

(Make the test callback async and await the builder result to unwrap the PromiseLike.)

— qwen3.7-max via Qwen Code /review


// Should have exactly 1 option (help flag)
Expand All @@ -35,9 +39,13 @@ describe('mcp command', () => {
version: vi.fn().mockReturnThis(),
};

mcpCommand.builder(mockYargs as unknown as Argv);
const builder = mcpCommand.builder;
if (typeof builder !== 'function') {
throw new Error('mcp command builder must be a function');
}
builder(mockYargs as unknown as Argv);

expect(mockYargs.command).toHaveBeenCalledTimes(4);
expect(mockYargs.command).toHaveBeenCalledTimes(6);

// Verify that the specific subcommands are registered
const commandCalls = mockYargs.command.mock.calls;
Expand All @@ -47,6 +55,8 @@ describe('mcp command', () => {
expect(commandNames).toContain('remove <name>');
expect(commandNames).toContain('list');
expect(commandNames).toContain('reconnect [server-name]');
expect(commandNames).toContain('approve [name]');
expect(commandNames).toContain('reject [name]');

expect(mockYargs.demandCommand).toHaveBeenCalledWith(
1,
Expand Down
3 changes: 3 additions & 0 deletions packages/cli/src/commands/mcp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { addCommand } from './mcp/add.js';
import { removeCommand } from './mcp/remove.js';
import { listCommand } from './mcp/list.js';
import { reconnectCommand } from './mcp/reconnect.js';
import { approveCommand, rejectCommand } from './mcp/approve.js';

export const mcpCommand: CommandModule = {
command: 'mcp',
Expand All @@ -20,6 +21,8 @@ export const mcpCommand: CommandModule = {
.command(removeCommand)
.command(listCommand)
.command(reconnectCommand)
.command(approveCommand)
.command(rejectCommand)
.demandCommand(1, 'You need at least one command before continuing.')
.version(false),
handler: () => {
Expand Down
146 changes: 146 additions & 0 deletions packages/cli/src/commands/mcp/approve.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,146 @@
/**
* @license
* Copyright 2025 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
import * as fs from 'node:fs';
import * as os from 'node:os';
import * as path from 'node:path';

const mockWriteStdoutLine = vi.hoisted(() => vi.fn());
vi.mock('../../utils/stdioHelpers.js', () => ({
writeStdoutLine: mockWriteStdoutLine,
writeStderrLine: vi.fn(),
clearScreen: vi.fn(),
}));

import { approveCommand, rejectCommand } from './approve.js';
import {
loadMcpApprovals,
resetMcpApprovalsForTesting,
MCP_APPROVALS_FILENAME,
} from '../../config/mcpApprovals.js';
import { loadProjectMcpServers } from '../../config/mcpJson.js';

describe('qwen mcp approve / reject', () => {
let dir: string;
let cwdSpy: ReturnType<typeof vi.spyOn>;

const output = () =>
mockWriteStdoutLine.mock.calls.map((c) => c[0]).join('\n');

const run = async (
cmd: typeof approveCommand,
argv: Record<string, unknown>,
) => {
await (cmd.handler as (a: Record<string, unknown>) => Promise<void>)({
_: [],
$0: 'qwen',
...argv,
});
};

beforeEach(() => {
dir = fs.mkdtempSync(path.join(os.tmpdir(), 'mcp-approve-'));
process.env['QWEN_CODE_MCP_APPROVALS_PATH'] = path.join(
dir,
MCP_APPROVALS_FILENAME,
);
resetMcpApprovalsForTesting();
cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(dir);
mockWriteStdoutLine.mockClear();
});

afterEach(() => {
delete process.env['QWEN_CODE_MCP_APPROVALS_PATH'];
resetMcpApprovalsForTesting();
cwdSpy.mockRestore();
fs.rmSync(dir, { recursive: true, force: true });
});

const writeMcpJson = (servers: Record<string, unknown>) =>
fs.writeFileSync(
path.join(dir, '.mcp.json'),
JSON.stringify({ mcpServers: servers }),
);

const stateOf = (name: string) => {
resetMcpApprovalsForTesting();
const { servers } = loadProjectMcpServers(dir);
return loadMcpApprovals().getState(dir, name, servers[name]!);
};

const writeWorkspaceSettings = (servers: Record<string, unknown>) => {
const qwenDir = path.join(dir, '.qwen');
fs.mkdirSync(qwenDir, { recursive: true });
fs.writeFileSync(
path.join(qwenDir, 'settings.json'),
JSON.stringify({ mcpServers: servers }),
);
};

/** Read the persisted approval status straight off disk (scope-agnostic). */
const persistedStatus = (name: string): string | undefined => {
const raw = fs.readFileSync(
process.env['QWEN_CODE_MCP_APPROVALS_PATH']!,
'utf-8',
);
return JSON.parse(raw)[dir]?.[name]?.status;
};

it('reports when there are no gated servers', async () => {
await run(approveCommand, { name: 'slack', all: false });
expect(output()).toContain('No approval-requiring MCP servers found');
});

it('approves a named project server (pending -> approved)', async () => {
writeMcpJson({ slack: { command: 'node', args: ['slack.js'] } });
expect(stateOf('slack')).toBe('pending');

await run(approveCommand, { name: 'slack', all: false });

expect(stateOf('slack')).toBe('approved');
expect(output()).toContain('Approved MCP server "slack"');
});

it('approves a workspace .qwen/settings.json server', async () => {
writeWorkspaceSettings({ ws: { command: 'node', args: ['ws.js'] } });

await run(approveCommand, { name: 'ws', all: false });

expect(output()).toContain('Approved MCP server "ws"');
expect(persistedStatus('ws')).toBe('approved');
});

it('rejects a named project server', async () => {
writeMcpJson({ slack: { command: 'node' } });
await run(rejectCommand, { name: 'slack', all: false });
expect(stateOf('slack')).toBe('rejected');
});

it('approves all with --all', async () => {
writeMcpJson({ a: { command: 'a' }, b: { command: 'b' } });
await run(approveCommand, { name: undefined, all: true });
expect(stateOf('a')).toBe('approved');
expect(stateOf('b')).toBe('approved');
});

it('reports an unknown server name', async () => {
writeMcpJson({ slack: { command: 'node' } });
await run(approveCommand, { name: 'ghost', all: false });
expect(output()).toContain('not found');
expect(stateOf('slack')).toBe('pending');
});

it('binds approval to the config hash: editing .mcp.json reverts to pending', async () => {
writeMcpJson({ slack: { command: 'node', args: ['slack.js'] } });
await run(approveCommand, { name: 'slack', all: false });
expect(stateOf('slack')).toBe('approved');

// Edit the server's command — approval must no longer apply.
writeMcpJson({ slack: { command: 'curl', args: ['slack.js'] } });
expect(stateOf('slack')).toBe('pending');
});
});
132 changes: 132 additions & 0 deletions packages/cli/src/commands/mcp/approve.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
/**
* @license
* Copyright 2025 Google LLC
* SPDX-License-Identifier: Apache-2.0
*/

// Files for 'qwen mcp approve' / 'qwen mcp reject' commands (issue #4615).
import type { CommandModule } from 'yargs';
import type { MCPServerConfig } from '@qwen-code/qwen-code-core';
import { isGatedMcpScope } from '@qwen-code/qwen-code-core';
import { writeStdoutLine } from '../../utils/stdioHelpers.js';
import { loadSettings } from '../../config/settings.js';
import { assembleMcpServers } from '../../config/mcpServers.js';
import {
loadMcpApprovals,
type McpApprovalStatus,
} from '../../config/mcpApprovals.js';

/**
* All gated (approval-requiring) servers visible from `cwd` — project
* `.mcp.json` plus workspace `.qwen/settings.json` (#4615). Non-gated sources
* (user/system/extension) never need approval and are excluded.
*/
function loadGatedServers(cwd: string): Record<string, MCPServerConfig> {
const settings = loadSettings(cwd);
const all = assembleMcpServers(settings.merged.mcpServers, cwd);
const gated: Record<string, MCPServerConfig> = {};
for (const [serverName, config] of Object.entries(all)) {
if (isGatedMcpScope(config.scope)) {
gated[serverName] = config;
}
}
return gated;
}

async function setProjectServerStatus(
name: string | undefined,
status: McpApprovalStatus,
all: boolean,
): Promise<void> {
const cwd = process.cwd();
const servers = loadGatedServers(cwd);

const names = Object.keys(servers);
if (names.length === 0) {
writeStdoutLine(
'No approval-requiring MCP servers found (looked in .mcp.json and .qwen/settings.json).',
);
return;
}

const verb = status === 'approved' ? 'Approved' : 'Rejected';
const approvals = loadMcpApprovals();

const targets = all ? names : name ? [name] : [];
if (targets.length === 0) {
writeStdoutLine('Specify a server name or pass --all.');
return;
}

for (const target of targets) {
const config = servers[target];
if (!config) {
writeStdoutLine(
`Server "${target}" not found. Available: ${names.join(', ')}`,
);
continue;
}
// The decision binds to this exact config's hash: editing the server in
// its source file later returns it to pending (issue #4615).
await approvals.setState(cwd, target, config, status);
writeStdoutLine(
`${verb} MCP server "${target}" (bound to its current config).`,
);
}

if (status === 'approved') {
writeStdoutLine(
'Approved servers connect in your next interactive session.',
);
}
}

export const approveCommand: CommandModule = {
command: 'approve [name]',
describe:
'Approve a gated MCP server (.mcp.json or workspace .qwen/settings.json)',
builder: (yargs) =>
yargs
.usage('Usage: qwen mcp approve [options] [name]')
.positional('name', {
describe: 'Name of the gated server to approve',
type: 'string',
})
.option('all', {
describe: 'Approve all gated servers in this workspace',
type: 'boolean',
default: false,
}),
handler: async (argv) => {
await setProjectServerStatus(
argv['name'] as string | undefined,
'approved',
argv['all'] as boolean,
);
},
};

export const rejectCommand: CommandModule = {
command: 'reject [name]',
describe:
'Reject a gated MCP server (.mcp.json or workspace .qwen/settings.json)',
builder: (yargs) =>
yargs
.usage('Usage: qwen mcp reject [options] [name]')
.positional('name', {
describe: 'Name of the gated server to reject',
type: 'string',
})
.option('all', {
describe: 'Reject all gated servers in this workspace',
type: 'boolean',
default: false,
}),
handler: async (argv) => {
await setProjectServerStatus(
argv['name'] as string | undefined,
'rejected',
argv['all'] as boolean,
);
},
};
Loading
Loading