Skip to content

fix: scope env warnings to selected server #245

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

Open
haosenwang1018 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: scope env warnings to selected server #245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions src/commands/info.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,15 @@ function parseTarget(target: string): { server: string; tool?: string } {
export async function infoCommand(options: InfoOptions): Promise<void> {
let config: McpServersConfig;

const { server: serverName, tool: toolName } = parseTarget(options.target);

try {
config = await loadConfig(options.configPath);
config = await loadConfig(options.configPath, [serverName]);
} catch (error) {
console.error((error as Error).message);
process.exit(ErrorCode.CLIENT_ERROR);
}

const { server: serverName, tool: toolName } = parseTarget(options.target);

let serverConfig: ServerConfig;
try {
serverConfig = getServerConfig(config, serverName);
Expand Down
23 changes: 21 additions & 2 deletions src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,6 +398,7 @@ function getDefaultConfigPaths(): string[] {
*/
export async function loadConfig(
explicitPath?: string,
serverNames?: string[],
): Promise<McpServersConfig> {
let configPath: string | undefined;

Expand Down Expand Up @@ -496,8 +497,26 @@ export async function loadConfig(
}
}

// Substitute environment variables
config = substituteEnvVarsInObject(config);
// Substitute environment variables only for the requested server(s)
// when a command has already narrowed the target. This avoids emitting
// missing-env warnings for unrelated servers.
if (serverNames && serverNames.length > 0) {
const scopedServers = new Set(serverNames);
const substitutedServers = Object.fromEntries(
Object.entries(config.mcpServers).map(([name, serverConfig]) => [
name,
scopedServers.has(name)
? substituteEnvVarsInObject(serverConfig)
: serverConfig,
]),
);
config = {
...config,
mcpServers: substitutedServers,
};
} else {
config = substituteEnvVarsInObject(config);
}

return config;
}
Expand Down
93 changes: 67 additions & 26 deletions tests/config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,16 @@
* Unit tests for config module
*/

import { describe, test, expect, beforeEach, afterEach } from 'bun:test';
import { mkdtemp, writeFile, rm } from 'node:fs/promises';
import { afterEach, beforeEach, describe, expect, spyOn, test } from 'bun:test';
import { mkdtemp, rm, writeFile } from 'node:fs/promises';
import { tmpdir } from 'node:os';
import { join } from 'node:path';
import {
loadConfig,
getServerConfig,
listServerNames,
isHttpServer,
isStdioServer,
listServerNames,
loadConfig,
} from '../src/config';

describe('config', () => {
Expand All @@ -34,12 +34,14 @@ describe('config', () => {
mcpServers: {
test: { command: 'echo', args: ['hello'] },
},
})
}),
);

const config = await loadConfig(configPath);
expect(config.mcpServers.test).toBeDefined();
expect((config.mcpServers.test as any).command).toBe('echo');
expect(
isStdioServer(config.mcpServers.test) && config.mcpServers.test.command,
).toBe('echo');
});

test('throws on missing config file', async () => {
Expand Down Expand Up @@ -74,14 +76,16 @@ describe('config', () => {
headers: { Authorization: 'Bearer ${TEST_MCP_TOKEN}' },
},
},
})
}),
);

const config = await loadConfig(configPath);
const server = config.mcpServers.test as any;
expect(server.headers.Authorization).toBe('Bearer secret123');
const server = config.mcpServers.test;
expect(isHttpServer(server) && server.headers?.Authorization).toBe(
'Bearer secret123',
);

delete process.env.TEST_MCP_TOKEN;
process.env.TEST_MCP_TOKEN = undefined;
});

test('handles missing env vars gracefully with MCP_STRICT_ENV=false', async () => {
Expand All @@ -98,19 +102,19 @@ describe('config', () => {
env: { TOKEN: '${NONEXISTENT_VAR}' },
},
},
})
}),
);

const config = await loadConfig(configPath);
const server = config.mcpServers.test as any;
expect(server.env.TOKEN).toBe('');
const server = config.mcpServers.test;
expect(isStdioServer(server) && server.env?.TOKEN).toBe('');

delete process.env.MCP_STRICT_ENV;
process.env.MCP_STRICT_ENV = undefined;
});

test('throws error on missing env vars in strict mode (default)', async () => {
// Ensure strict mode is enabled (default)
delete process.env.MCP_STRICT_ENV;
process.env.MCP_STRICT_ENV = undefined;

const configPath = join(tempDir, 'missing_env_strict.json');
await writeFile(
Expand All @@ -122,12 +126,43 @@ describe('config', () => {
env: { TOKEN: '${ANOTHER_NONEXISTENT_VAR}' },
},
},
})
}),
);

await expect(loadConfig(configPath)).rejects.toThrow('MISSING_ENV_VAR');
});

test('only substitutes env vars for the requested server scope', async () => {
process.env.MCP_STRICT_ENV = 'false';
const stderrSpy = spyOn(console, 'error').mockImplementation(() => {});

const configPath = join(tempDir, 'scoped_missing_env.json');
await writeFile(
configPath,
JSON.stringify({
mcpServers: {
chrome: { command: 'echo', args: ['ok'] },
email: {
command: 'echo',
env: { TOKEN: '${EMAIL_TOKEN}' },
},
},
}),
);

const config = await loadConfig(configPath, ['chrome']);
expect(
isStdioServer(config.mcpServers.chrome) &&
config.mcpServers.chrome.command,
).toBe('echo');
expect(stderrSpy).not.toHaveBeenCalledWith(
expect.stringContaining('${EMAIL_TOKEN}'),
);

stderrSpy.mockRestore();
process.env.MCP_STRICT_ENV = undefined;
});

test('throws error on empty server config', async () => {
const configPath = join(tempDir, 'empty_server.json');
await writeFile(
Expand All @@ -136,10 +171,12 @@ describe('config', () => {
mcpServers: {
badserver: {},
},
})
}),
);

await expect(loadConfig(configPath)).rejects.toThrow('missing required field');
await expect(loadConfig(configPath)).rejects.toThrow(
'missing required field',
);
});

test('throws error on server with both command and url', async () => {
Expand All @@ -153,10 +190,12 @@ describe('config', () => {
url: 'https://example.com',
},
},
})
}),
);

await expect(loadConfig(configPath)).rejects.toThrow('both "command" and "url"');
await expect(loadConfig(configPath)).rejects.toThrow(
'both "command" and "url"',
);
});

test('throws error on null server config', async () => {
Expand All @@ -167,10 +206,12 @@ describe('config', () => {
mcpServers: {
nullserver: null,
},
})
}),
);

await expect(loadConfig(configPath)).rejects.toThrow('Invalid server configuration');
await expect(loadConfig(configPath)).rejects.toThrow(
'Invalid server configuration',
);
});
});

Expand All @@ -184,12 +225,12 @@ describe('config', () => {
server1: { command: 'cmd1' },
server2: { command: 'cmd2' },
},
})
}),
);

const config = await loadConfig(configPath);
const server = getServerConfig(config, 'server1');
expect((server as any).command).toBe('cmd1');
expect(isStdioServer(server) && server.command).toBe('cmd1');
});

test('throws on unknown server', async () => {
Expand All @@ -198,7 +239,7 @@ describe('config', () => {
configPath,
JSON.stringify({
mcpServers: { known: { command: 'cmd' } },
})
}),
);

const config = await loadConfig(configPath);
Expand All @@ -217,7 +258,7 @@ describe('config', () => {
beta: { command: 'b' },
gamma: { url: 'https://example.com' },
},
})
}),
);

const config = await loadConfig(configPath);
Expand Down