From 2f9d7b9ca9e1c9f0e9bab7d61b29d27532354d26 Mon Sep 17 00:00:00 2001 From: Nikolay Vitkov Date: Fri, 25 Sep 2026 15:41:39 +0000 Subject: [PATCH 1/3] feat: resolve config defaults after merging CLI and config file Parse CLI arguments and the JSON config file without yargs defaults so only explicit user intent is captured, merge them with CLI taking precedence, then apply defaults (including the --viaCli ones). Conflict checks now only consider explicitly provided arguments, so implicit defaults such as channel=stable can no longer conflict with explicit flags. Also restores --help/--version exiting via a shared buildCliParser and adds edge-case tests for config parsing, precedence and conflicts. --- src/config/browser-options.ts | 4 +- src/config/mcp-options.ts | 313 +++++++++++++++++++------------ tests/cli.test.ts | 281 +++++++++++++++++++++++---- tests/config/mcp-options.test.ts | 142 ++++++++++++++ tests/mocks.ts | 12 +- 5 files changed, 595 insertions(+), 157 deletions(-) create mode 100644 tests/config/mcp-options.test.ts diff --git a/src/config/browser-options.ts b/src/config/browser-options.ts index 2b9af492d..d824301d3 100644 --- a/src/config/browser-options.ts +++ b/src/config/browser-options.ts @@ -91,7 +91,7 @@ export const browserOptions = { type: 'boolean', description: 'If specified, creates a temporary user-data-dir that is automatically cleaned up after the browser is closed. Defaults to false.', - defaultDescription: 'false', + default: false, }, userDataDir: { type: 'string', @@ -103,7 +103,7 @@ export const browserOptions = { description: 'Specify a different Chrome channel that should be used. The default is the stable channel version.', choices: ['canary', 'dev', 'beta', 'stable'] as const, - defaultDescription: 'stable', + default: 'stable' as const, }, proxyServer: { type: 'string', diff --git a/src/config/mcp-options.ts b/src/config/mcp-options.ts index 292bc7ad0..22466d5fb 100644 --- a/src/config/mcp-options.ts +++ b/src/config/mcp-options.ts @@ -144,7 +144,7 @@ export const mcpOptions = { describe: "Restricts browser's network access by blocking specified URL patterns (uses https://urlpattern.spec.whatwg.org/). Silently detaches from targets with blocked URLs upon connection, and blocks runtime requests (including navigations and subresources). Accepts an array of patterns. A pattern that uses a regexp group in any component (for example `(127\\.\\d+\\.\\d+\\.\\d+)` in the hostname) is rejected, because it is not enforced on redirects or subresources; use an exact value or a `*`/`:name` wildcard instead.", coerce: (arg: string[] | undefined) => { - if (arg === undefined) { + if (arg === undefined || arg.length === 0) { return undefined; } const pattern = findUnenforceablePattern(arg); @@ -165,6 +165,11 @@ export const mcpOptions = { if (arg === undefined) { return undefined; } + if (arg.length === 0) { + throw new Error( + 'Invalid --allowedUrlPattern: at least one pattern is required.', + ); + } const pattern = findUnenforceablePattern(arg); if (pattern) { throw new Error( @@ -319,9 +324,14 @@ export const mcpOptions = { }, } satisfies Record; -export type ParsedArguments = ReturnType; +export type ParsedArguments = ReturnType< + ReturnType>['parseSync'] +>; -export function getMcpOptionsForViaCli(): typeof mcpOptions { +export function getMcpOptionsForViaCli(): Record< + keyof typeof mcpOptions, + YargsOptions +> { if (!('default' in mcpOptions.headless)) { throw new Error('headless cli option unexpectedly does not have a default'); } @@ -473,18 +483,13 @@ function getErrorMessage(err: unknown): string { return err instanceof Error ? err.message : String(err); } -/** - * Exported only for testing to not trigger process exit. - */ -export function parser( +export function buildCliParser>( version: string, - argv = process.argv, - env = process.env, + argv: string[], + options: T, ) { - const isViaCli = argv.includes('--viaCli') || argv.includes('--via-cli'); - const options = isViaCli ? getMcpOptionsForViaCli() : mcpOptions; - - const yargsInstance = yargs(hideBin(argv)) + const yargsInstance = yargs(hideBin(argv)); + return yargsInstance .scriptName('npx chrome-devtools-mcp@latest') .parserConfiguration({ 'strip-aliased': true, @@ -492,126 +497,194 @@ export function parser( }) .options(options) .showHelpOnFail(false, 'Specify --help for available options') - .check(args => { - const activeArgs = new Set(); + .example(CLI_EXAMPLES) + .wrap(Math.min(120, yargsInstance.terminalWidth())) + .help() + .version(version); +} - for (const [key, val] of Object.entries(args)) { - if (val !== undefined && val !== false) { - activeArgs.add(key); - } - } +function withoutDefaults( + options: Record, +): Record { + const result: Record = {}; + for (const [key, option] of Object.entries(options)) { + const copy: YargsOptions = {...option}; + if (copy.default !== undefined) { + copy.defaultDescription ??= JSON.stringify(copy.default); + delete copy.default; + } + result[key] = copy; + } + return result; +} - for (const group of CONFLICTING_ARGS) { - // Find all active arguments within this conflict group - const activeInGroup = group.filter(arg => activeArgs.has(arg)); +function stripYargsPositionalArgs( + parsed: Partial, +): Partial { + delete (parsed as {_?: unknown})._; + delete (parsed as {$0?: unknown}).$0; + return parsed; +} - if (activeInGroup.length > 1) { - const [arg1, arg2] = activeInGroup; - throw new Error( - `Arguments ${arg1} and ${arg2} are mutually exclusive`, - ); - } - } +/** + * Step 1: parses the CLI flags without applying defaults, so the result only + * contains what the user passed. `--help` and `--version` print and exit the + * process; parse errors throw. + */ +export function parseCliArgs( + version: string, + argv: string[], +): Partial { + const parsed = buildCliParser(version, argv, withoutDefaults(mcpOptions)) + .fail(false) + .parseSync(); + return stripYargsPositionalArgs(parsed); +} - return true; - }) - .middleware(args => { - if (isViaCli) { - if (args.filesystemRoot === DEFAULT_FILESYSTEM_ROOT) { - const cliFilesystemArgs: { - allowUnrestrictedPaths?: boolean; - filesystemRoot?: unknown; - } = args; - cliFilesystemArgs.allowUnrestrictedPaths = true; - cliFilesystemArgs.filesystemRoot = undefined; - } - // Defaults that cannot be set in options without affecting yargs conflict resolution. - const connectsToExistingBrowser = - args.autoConnect || args.browserUrl || args.wsEndpoint; - if ( - args.isolated === undefined && - args.userDataDir === undefined && - !connectsToExistingBrowser - ) { - args.isolated = true; - } - if ( - args.categoryExtensions === undefined && - !connectsToExistingBrowser - ) { - args.categoryExtensions = true; - } - } - // Only fall back to stable when Chrome is launched by channel. Leaving it - // unset otherwise keeps it out of telemetry (computeFlagUsage) for - // browserUrl, wsEndpoint and executablePath. - if ( - !args.channel && - !args.browserUrl && - !args.wsEndpoint && - !args.executablePath - ) { - args.channel = 'stable'; - } - if (env['CI'] || env['CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS']) { - console.error( - "turning off usage statistics. process.env['CI'] || process.env['CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS'] is set.", - ); - args.usageStatistics = false; - } +/** + * Step 2: reads the JSON config file and runs it through yargs to reject + * unknown keys and apply coercions, without applying defaults. + */ +export function parseConfigFile(configPath: string): Partial { + try { + const fileContent: unknown = JSON.parse(readFileSync(configPath, 'utf-8')); + if (!isPlainObject(fileContent)) { + throw new Error('Config must be a JSON object'); + } + const parsed = yargs([]) + .parserConfiguration({ + 'strip-aliased': true, + 'camel-case-expansion': false, + }) + .options(withoutDefaults(mcpOptions)) + .config(fileContent) + .strict() + .fail(false) + .exitProcess(false) + .parseSync([]); + return stripYargsPositionalArgs(parsed); + } catch (err) { + throw new Error(`Invalid JSON config file: ${getErrorMessage(err)}`); + } +} - const cliOptionsAllowedArgs = [ - ...Object.keys(options), - // Yargs populated with positional args - '_', - '$0', - ]; +function warnUnknownArgs(cliArgs: Partial): void { + const allowedArgs = new Set(Object.keys(mcpOptions)); + const unknownArgs = Object.keys(cliArgs).filter(arg => !allowedArgs.has(arg)); + if (unknownArgs.length > 0) { + console.error(`Unknown arguments: ${unknownArgs.map(arg => `--${arg}`)}`); + } +} - const unknownArgs = Object.keys(args).filter( - arg => !cliOptionsAllowedArgs.includes(arg), - ); +/** + * Step 4: rejects mutually exclusive inputs. Only explicit inputs are checked, + * so defaults never conflict. + */ +export function validateConflicts( + explicitArgs: Partial, +): void { + const activeArgs = new Set(); + for (const [key, val] of Object.entries(explicitArgs)) { + if (val !== undefined && val !== false) { + activeArgs.add(key); + } + } + for (const group of CONFLICTING_ARGS) { + const activeInGroup = group.filter(arg => activeArgs.has(arg)); + if (activeInGroup.length > 1) { + const [arg1, arg2] = activeInGroup; + throw new Error(`Arguments ${arg1} and ${arg2} are mutually exclusive`); + } + } +} - if (unknownArgs.length > 0) { - console.error( - `Unknown arguments: ${unknownArgs.map(arg => `--${arg}`)}`, - ); - } - }) - .example(CLI_EXAMPLES); +/** + * Step 5: fills in defaults for everything that was not set explicitly. + * `viaCli` is an explicit input like any other; it selects which defaults + * apply. + */ +export function applyDefaults( + explicitArgs: Partial, + env: NodeJS.ProcessEnv, +): Partial { + const isViaCli = explicitArgs.viaCli === true; + const baseOptions = isViaCli ? getMcpOptionsForViaCli() : mcpOptions; + const resolvedArgs = {...explicitArgs}; + // `channel` only applies when Chrome is launched by channel. Leaving it + // unset otherwise keeps it out of telemetry (computeFlagUsage). + const launchesByChannel = + !explicitArgs.browserUrl && + !explicitArgs.wsEndpoint && + !explicitArgs.executablePath; - return yargsInstance - .config('config', 'Path to JSON configuration file', configPath => { - try { - const parsed: unknown = JSON.parse(readFileSync(configPath, 'utf-8')); - if (!isPlainObject(parsed)) { - throw new Error('Config must be a JSON object'); - } + for (const [key, option] of Object.entries(baseOptions)) { + if (key === 'channel' && !launchesByChannel) { + continue; + } + if (resolvedArgs[key] === undefined && 'default' in option) { + resolvedArgs[key] = option.default; + } + } - yargs() - .parserConfiguration({ - 'strip-aliased': true, - 'camel-case-expansion': false, - }) - .options(options) - .config(parsed) - .strict() - .fail(false) - .exitProcess(false) - .parseSync([]); - return parsed; - } catch (err) { - throw new Error(`Invalid JSON config file: ${getErrorMessage(err)}`); - } - }) - .wrap(Math.min(120, yargsInstance.terminalWidth())) - .help() - .version(version); + if (isViaCli) { + if (resolvedArgs.filesystemRoot === DEFAULT_FILESYSTEM_ROOT) { + resolvedArgs.allowUnrestrictedPaths = true; + resolvedArgs.filesystemRoot = undefined; + } + const connectsToExistingBrowser = + resolvedArgs.autoConnect || + resolvedArgs.browserUrl || + resolvedArgs.wsEndpoint; + if ( + explicitArgs.isolated === undefined && + resolvedArgs.userDataDir === undefined && + !connectsToExistingBrowser + ) { + resolvedArgs.isolated = true; + } + if ( + resolvedArgs.categoryExtensions === undefined && + !connectsToExistingBrowser + ) { + resolvedArgs.categoryExtensions = true; + } + } + + if (env['CI'] || env['CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS']) { + console.error( + "turning off usage statistics. process.env['CI'] || process.env['CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS'] is set.", + ); + resolvedArgs.usageStatistics = false; + } + + return resolvedArgs; } export function parseArguments( version: string, argv = process.argv, env = process.env, -) { - return parser(version, argv, env).parseSync(); + exitProcess = true, +): ParsedArguments { + try { + const cliArgs = parseCliArgs(version, argv); + const configFileArgs = + typeof cliArgs.config === 'string' ? parseConfigFile(cliArgs.config) : {}; + // Step 3: merges the explicit inputs. The CLI wins over the config file. + const explicitArgs = {...configFileArgs, ...cliArgs}; + warnUnknownArgs(cliArgs); + validateConflicts(explicitArgs); + const resolvedArgs = applyDefaults(explicitArgs, env); + + // The merge and the default loop lose the static type that yargs infers from + // `mcpOptions`. Every value was produced by the same option definitions (CLI + // parser, strict config-file parser, option defaults), so the shape matches. + return resolvedArgs as ParsedArguments; + } catch (error) { + if (exitProcess) { + console.error(getErrorMessage(error)); + process.exit(1); + } + throw error; + } } diff --git a/tests/cli.test.ts b/tests/cli.test.ts index 7d63b7bf4..41e960333 100644 --- a/tests/cli.test.ts +++ b/tests/cli.test.ts @@ -11,19 +11,18 @@ import {describe, it} from 'node:test'; import {buildCommand} from '../src/config/cli-commands.js'; import {commands} from '../src/config/cli-options.js'; import { + buildCliParser, DEFAULT_FILESYSTEM_ROOT, getCliOptions, mcpOptions, - parser, + parseArguments as parseArgumentsImpl, } from '../src/config/mcp-options.js'; import {computeFlagUsage} from '../src/telemetry/flagUtils.js'; import {createTempFile} from './utils.js'; function parseArguments(argv: string[], env: NodeJS.ProcessEnv = {}) { - return parser('0.0.0', ['node', 'main.js', ...argv], env) - .exitProcess(false) - .parseSync(); + return parseArgumentsImpl('0.0.0', ['node', 'main.js', ...argv], env, false); } describe('cli args parsing', () => { @@ -37,6 +36,7 @@ describe('cli args parsing', () => { categoryMemory: true, autoConnect: false, headless: false, + isolated: false, acceptInsecureCerts: false, performanceCrux: true, usageStatistics: true, @@ -65,8 +65,6 @@ describe('cli args parsing', () => { const args = parseArguments([]); assert.deepStrictEqual(args, { ...defaultArgs, - _: [], - $0: 'npx chrome-devtools-mcp@latest', channel: 'stable', }); }); @@ -86,8 +84,6 @@ describe('cli args parsing', () => { const args = parseArguments(['--browserUrl', 'http://localhost:3000']); assert.deepStrictEqual(args, { ...defaultArgs, - _: [], - $0: 'npx chrome-devtools-mcp@latest', browserUrl: 'http://localhost:3000', }); }); @@ -116,8 +112,6 @@ describe('cli args parsing', () => { const args = parseArguments(['--user-data-dir', '/tmp/chrome-profile']); assert.deepStrictEqual(args, { ...defaultArgs, - _: [], - $0: 'npx chrome-devtools-mcp@latest', channel: 'stable', userDataDir: '/tmp/chrome-profile', }); @@ -127,8 +121,6 @@ describe('cli args parsing', () => { const args = parseArguments(['--browserUrl', ''], {}); assert.deepStrictEqual(args, { ...defaultArgs, - _: [], - $0: 'npx chrome-devtools-mcp@latest', channel: 'stable', browserUrl: undefined, }); @@ -138,8 +130,6 @@ describe('cli args parsing', () => { const args = parseArguments(['--executablePath', '/tmp/test 123/chrome']); assert.deepStrictEqual(args, { ...defaultArgs, - _: [], - $0: 'npx chrome-devtools-mcp@latest', executablePath: '/tmp/test 123/chrome', }); }); @@ -148,8 +138,6 @@ describe('cli args parsing', () => { const args = parseArguments(['--viewport', '888x777']); assert.deepStrictEqual(args, { ...defaultArgs, - _: [], - $0: 'npx chrome-devtools-mcp@latest', channel: 'stable', viewport: { width: 888, @@ -165,8 +153,6 @@ describe('cli args parsing', () => { ]); assert.deepStrictEqual(args, { ...defaultArgs, - _: [], - $0: 'npx chrome-devtools-mcp@latest', channel: 'stable', chromeArg: ['--no-sandbox', '--disable-setuid-sandbox'], }); @@ -219,8 +205,6 @@ describe('cli args parsing', () => { ]); assert.deepStrictEqual(args, { ...defaultArgs, - _: [], - $0: 'npx chrome-devtools-mcp@latest', channel: 'stable', ignoreDefaultChromeArg: [ '--disable-extensions', @@ -236,8 +220,6 @@ describe('cli args parsing', () => { ]); assert.deepStrictEqual(args, { ...defaultArgs, - _: [], - $0: 'npx chrome-devtools-mcp@latest', wsEndpoint: 'ws://127.0.0.1:9222/devtools/browser/abc123', }); }); @@ -249,8 +231,6 @@ describe('cli args parsing', () => { ]); assert.deepStrictEqual(args, { ...defaultArgs, - _: [], - $0: 'npx chrome-devtools-mcp@latest', wsEndpoint: 'wss://example.com:9222/devtools/browser/abc123', }); }); @@ -272,8 +252,6 @@ describe('cli args parsing', () => { const args = parseArguments(['--no-category-emulation']); assert.deepStrictEqual(args, { ...defaultArgs, - _: [], - $0: 'npx chrome-devtools-mcp@latest', channel: 'stable', categoryEmulation: false, }); @@ -282,8 +260,6 @@ describe('cli args parsing', () => { const args = parseArguments(['--auto-connect'], {}); assert.deepStrictEqual(args, { ...defaultArgs, - _: [], - $0: 'npx chrome-devtools-mcp@latest', channel: 'stable', autoConnect: true, }); @@ -470,6 +446,44 @@ describe('cli args parsing', () => { ); }); + it('strips an empty blocked-url-pattern', async () => { + const args = parseArguments(['--blocked-url-pattern']); + assert.strictEqual(args.blockedUrlPattern, undefined); + }); + + it('allows an empty config blockedUrlPattern with allowed-url-pattern', async () => { + using testConfig = createTempFile( + JSON.stringify({blockedUrlPattern: []}), + 'cd4a.test.config.empty-blocked.json', + ); + const args = parseArguments([ + '--config', + testConfig.path, + '--allowed-url-pattern', + 'https://a.com/*', + ]); + assert.strictEqual(args.blockedUrlPattern, undefined); + assert.deepStrictEqual(args.allowedUrlPattern, ['https://a.com/*']); + }); + + it('rejects an empty allowed-url-pattern', async () => { + assert.throws( + () => parseArguments(['--allowed-url-pattern']), + /Invalid --allowedUrlPattern: at least one pattern is required/, + ); + }); + + it('rejects an empty config allowedUrlPattern', async () => { + using testConfig = createTempFile( + JSON.stringify({allowedUrlPattern: []}), + 'cd4a.test.config.empty-allowed.json', + ); + assert.throws( + () => parseArguments(['--config', testConfig.path]), + /Invalid JSON config file: Invalid --allowedUrlPattern: at least one pattern is required/, + ); + }); + it('parses source-maps flag', async () => { const defaultParsed = parseArguments(['main.js']); assert.strictEqual(defaultParsed.sourceMaps, true); @@ -561,7 +575,7 @@ describe('cli args parsing', () => { const args = parseArguments(['--viaCli', '--config', relativePath]); assert.strictEqual(args.config, testConfig.path); assert.strictEqual(args.userDataDir, '/tmp/custom-profile'); - assert.strictEqual(args.isolated, undefined); + assert.strictEqual(args.isolated, false); assert.strictEqual(args.headless, false); }); @@ -604,11 +618,145 @@ describe('cli args parsing', () => { ); }); + it('rejects a config file with malformed JSON', async () => { + using testConfig = createTempFile( + '{"headless": true,', + 'cd4a.test.config.malformed.json', + ); + assert.throws( + () => parseArguments(['--config', testConfig.path]), + /Invalid JSON config file:/, + ); + }); + + it('rejects a config file that is not a JSON object', async () => { + using testConfig = createTempFile( + JSON.stringify(['--headless']), + 'cd4a.test.config.array.json', + ); + assert.throws( + () => parseArguments(['--config', testConfig.path]), + /Invalid JSON config file: Config must be a JSON object/, + ); + }); + + it('rejects a missing config file', async () => { + assert.throws( + () => parseArguments(['--config', 'cd4a.test.config.missing.json']), + /Invalid JSON config file: ENOENT/, + ); + }); + + it('replaces config arrays with cli arrays instead of merging', async () => { + using testConfig = createTempFile( + JSON.stringify({chromeArg: ['--a']}), + 'cd4a.test.config.array-replace.json', + ); + const args = parseArguments([ + '--config', + testConfig.path, + '--chrome-arg=--b', + ]); + assert.deepStrictEqual(args.chromeArg, ['--b']); + }); + + it('lets the CI env disable usage statistics enabled in config', async () => { + using testConfig = createTempFile( + JSON.stringify({usageStatistics: true}), + 'cd4a.test.config.usage-statistics.json', + ); + const args = parseArguments(['--config', testConfig.path], {CI: 'true'}); + assert.strictEqual(args.usageStatistics, false); + }); + + it('lets explicit cli flags override viaCli dynamic defaults', async () => { + const args = parseArguments(['--viaCli', '--no-headless']); + assert.strictEqual(args.headless, false); + }); + + describe('viaCli defaults', () => { + for (const flag of ['--viaCli=true', '--via-cli=true']) { + it(`applies viaCli defaults for ${flag}`, async () => { + const args = parseArguments([flag]); + assert.strictEqual(args.viaCli, true); + assert.strictEqual(args.headless, true); + assert.strictEqual(args.isolated, true); + assert.strictEqual(args.memoryDebugging, true); + }); + } + + it('does not apply viaCli defaults for --viaCli false', async () => { + const args = parseArguments(['--viaCli', 'false']); + assert.strictEqual(args.viaCli, false); + assert.strictEqual(args.headless, false); + assert.strictEqual(args.isolated, false); + assert.strictEqual(args.memoryDebugging, false); + }); + + it('applies viaCli defaults when viaCli is set in the config file', async () => { + using testConfig = createTempFile( + JSON.stringify({viaCli: true}), + 'cd4a.test.config.via-cli.json', + ); + const args = parseArguments(['--config', testConfig.path]); + assert.strictEqual(args.viaCli, true); + assert.strictEqual(args.headless, true); + assert.strictEqual(args.isolated, true); + }); + + it('lets cli viaCli=false override config viaCli', async () => { + using testConfig = createTempFile( + JSON.stringify({viaCli: true}), + 'cd4a.test.config.via-cli-override.json', + ); + const args = parseArguments([ + '--config', + testConfig.path, + '--viaCli=false', + ]); + assert.strictEqual(args.viaCli, false); + assert.strictEqual(args.headless, false); + assert.strictEqual(args.isolated, false); + }); + + for (const connectArgs of [ + ['--auto-connect'], + ['--browserUrl', 'http://localhost:9222'], + ['--wsEndpoint', 'ws://localhost:9222'], + ['--user-data-dir', '/tmp/chrome-profile'], + ]) { + it(`does not default to isolated with ${connectArgs[0]}`, async () => { + const args = parseArguments(['--viaCli', ...connectArgs]); + assert.strictEqual(args.isolated, false); + }); + } + + it('does not default to isolated with autoConnect from the config file', async () => { + using testConfig = createTempFile( + JSON.stringify({autoConnect: true}), + 'cd4a.test.config.via-cli-auto-connect.json', + ); + const args = parseArguments(['--viaCli', '--config', testConfig.path]); + assert.strictEqual(args.autoConnect, true); + assert.strictEqual(args.isolated, false); + }); + }); + it('parses with devtoolsComments enabled', async () => { const args = parseArguments(['--devtoolsComments']); assert.strictEqual(args.devtoolsComments, true); }); + it('includes usage examples in help output', async () => { + const help = await buildCliParser( + '0.0.0', + ['node', 'main.js'], + mcpOptions, + ).getHelp(); + assert.match(help, /Examples:/); + assert.match(help, /--browserUrl http:\/\/127\.0\.0\.1:9222/); + }); + it('clears default values and populates defaultDescription in getCliOptions', () => { const cliOptions = getCliOptions(); @@ -832,6 +980,73 @@ describe('cli args parsing', () => { ); }); + it('rejects cli channel with config-based browserUrl', async () => { + using testConfig = createTempFile( + JSON.stringify({browserUrl: 'http://localhost:9222'}), + 'cd4a.test.config.browser-url.json', + ); + assert.throws( + () => + parseArguments(['--config', testConfig.path, '--channel', 'canary']), + /Arguments channel and browserUrl are mutually exclusive/, + ); + }); + + it('rejects conflicting arguments within a config file', async () => { + using testConfig = createTempFile( + JSON.stringify({ + browserUrl: 'http://localhost:9222', + wsEndpoint: 'ws://localhost:9222', + }), + 'cd4a.test.config.conflict.json', + ); + assert.throws( + () => parseArguments(['--config', testConfig.path]), + /Arguments browserUrl and wsEndpoint are mutually exclusive/, + ); + }); + + it('allows explicitly disabled isolated with userDataDir', async () => { + const args = parseArguments([ + '--isolated=false', + '--user-data-dir', + '/tmp/chrome-profile', + ]); + assert.strictEqual(args.isolated, false); + assert.strictEqual(args.userDataDir, '/tmp/chrome-profile'); + }); + + it('allows a config value set to false alongside a conflicting arg', async () => { + using testConfig = createTempFile( + JSON.stringify({autoConnect: false}), + 'cd4a.test.config.false-no-conflict.json', + ); + const args = parseArguments([ + '--config', + testConfig.path, + '--executablePath', + '/bin/chrome', + ]); + assert.strictEqual(args.autoConnect, false); + assert.strictEqual(args.executablePath, '/bin/chrome'); + }); + + it('lets a cli false override a conflicting config value', async () => { + using testConfig = createTempFile( + JSON.stringify({autoConnect: true}), + 'cd4a.test.config.false-override.json', + ); + const args = parseArguments([ + '--config', + testConfig.path, + '--autoConnect=false', + '--executablePath', + '/bin/chrome', + ]); + assert.strictEqual(args.autoConnect, false); + assert.strictEqual(args.executablePath, '/bin/chrome'); + }); + it('rejects categoryExtensions with autoConnect', async () => { assert.throws( () => parseArguments(['--category-extensions', '--auto-connect']), @@ -882,7 +1097,7 @@ describe('cli args parsing', () => { it(`allows viaCli with ${connectArgs[0]} without enabling extensions`, async () => { const args = parseArguments(['--viaCli', ...connectArgs]); assert.strictEqual(args.categoryExtensions, undefined); - assert.strictEqual(args.isolated, undefined); + assert.strictEqual(args.isolated, false); }); } @@ -975,8 +1190,8 @@ describe('cli command strings', () => { describe('flag usage telemetry', () => { it('reports the stable channel for a default launch', async () => { const usage = computeFlagUsage(parseArguments([]), mcpOptions); - assert.strictEqual(usage.isolated_present, false); - assert.strictEqual(usage.channel_present, true); + assert.strictEqual(usage.isolated_present, undefined); + assert.strictEqual(usage.channel_present, undefined); assert.strictEqual(usage.channel, 'CHANNEL_STABLE'); }); @@ -987,7 +1202,7 @@ describe('flag usage telemetry', () => { ]) { it(`does not report channel with ${connectArgs[0]}`, async () => { const usage = computeFlagUsage(parseArguments(connectArgs), mcpOptions); - assert.strictEqual(usage.isolated_present, false); + assert.strictEqual(usage.isolated_present, undefined); assert.strictEqual(usage.channel_present, false); assert.strictEqual(usage.channel, undefined); }); diff --git a/tests/config/mcp-options.test.ts b/tests/config/mcp-options.test.ts new file mode 100644 index 000000000..e4de84963 --- /dev/null +++ b/tests/config/mcp-options.test.ts @@ -0,0 +1,142 @@ +/** + * @license + * Copyright 2026 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import assert from 'node:assert'; +import {afterEach, describe, it} from 'node:test'; + +import sinon from 'sinon'; + +import { + applyDefaults, + DEFAULT_FILESYSTEM_ROOT, + parseCliArgs, + parseConfigFile, + validateConflicts, +} from '../../src/config/mcp-options.js'; +import {createTempFile} from '../utils.js'; + +describe('mcp-options steps', () => { + afterEach(() => sinon.restore()); + + describe('parseCliArgs', () => { + it('returns only explicitly passed flags', () => { + const args = parseCliArgs('0.0.0', ['node', 'main.js', '--headless']); + assert.deepStrictEqual(args, { + headless: true, + }); + }); + }); + + describe('parseConfigFile', () => { + it('returns only the keys from the file', () => { + using configFile = createTempFile( + JSON.stringify({ + headless: true, + blockedUrlPattern: ['https://a.com/*'], + }), + 'cd4a.steps.config.json', + ); + const args = parseConfigFile(configFile.path); + assert.strictEqual(args.headless, true); + assert.deepStrictEqual(args.blockedUrlPattern, ['https://a.com/*']); + assert.strictEqual(args.isolated, undefined); + assert.strictEqual(args.channel, undefined); + }); + + it('rejects unknown keys', () => { + using configFile = createTempFile( + JSON.stringify({notAnOption: true}), + 'cd4a.steps.config.unknown.json', + ); + assert.throws( + () => parseConfigFile(configFile.path), + /Invalid JSON config file: .*notAnOption/, + ); + }); + + it('rejects non-object JSON', () => { + using configFile = createTempFile( + JSON.stringify([]), + 'cd4a.steps.config.array.json', + ); + assert.throws( + () => parseConfigFile(configFile.path), + /Invalid JSON config file: Config must be a JSON object/, + ); + }); + }); + + describe('validateConflicts', () => { + it('accepts a single argument from a conflict group', () => { + validateConflicts({browserUrl: 'http://localhost:9222'}); + }); + + it('ignores false and undefined values', () => { + validateConflicts({ + browserUrl: 'http://localhost:9222', + wsEndpoint: undefined, + categoryExtensions: false, + }); + }); + + it('rejects two arguments from the same group', () => { + assert.throws( + () => + validateConflicts({ + browserUrl: 'http://localhost:9222', + channel: 'canary', + }), + /Arguments channel and browserUrl are mutually exclusive/, + ); + }); + }); + describe('applyDefaults', () => { + it('keeps explicit values and fills in defaults', () => { + const args = applyDefaults({headless: true}, {}); + assert.strictEqual(args.headless, true); + assert.strictEqual(args.isolated, false); + assert.strictEqual(args.channel, 'stable'); + assert.strictEqual(args.categoryExtensions, undefined); + }); + + for (const explicitArgs of [ + {browserUrl: 'http://localhost:9222'}, + {wsEndpoint: 'ws://localhost:9222'}, + {executablePath: '/tmp/chrome'}, + ]) { + it(`does not default channel with ${Object.keys(explicitArgs)[0]}`, () => { + assert.strictEqual(applyDefaults(explicitArgs, {}).channel, undefined); + }); + } + + it('applies viaCli defaults when launching a browser', () => { + const args = applyDefaults( + {viaCli: true, filesystemRoot: DEFAULT_FILESYSTEM_ROOT}, + {}, + ); + assert.strictEqual(args.headless, true); + assert.strictEqual(args.isolated, true); + assert.strictEqual(args.categoryExtensions, true); + assert.strictEqual(args.allowUnrestrictedPaths, true); + assert.strictEqual(args.filesystemRoot, undefined); + }); + + it('does not enable isolated or extensions for viaCli with browserUrl', () => { + const args = applyDefaults( + {viaCli: true, browserUrl: 'http://localhost:9222'}, + {}, + ); + assert.strictEqual(args.isolated, false); + assert.strictEqual(args.categoryExtensions, undefined); + }); + + it('turns off usage statistics in CI', () => { + sinon.stub(console, 'error'); + const args = applyDefaults({usageStatistics: true}, {CI: 'true'}); + assert.strictEqual(args.usageStatistics, false); + }); + }); +}); diff --git a/tests/mocks.ts b/tests/mocks.ts index 4c09435e2..a009898de 100644 --- a/tests/mocks.ts +++ b/tests/mocks.ts @@ -25,7 +25,10 @@ import type {Frame} from 'puppeteer-core'; import sinon from 'sinon'; -import {type ParsedArguments, parser} from '../src/config/mcp-options.js'; +import { + type ParsedArguments, + parseArguments, +} from '../src/config/mcp-options.js'; import {McpContext} from '../src/McpContext.js'; import {McpPage} from '../src/McpPage.js'; import {McpResponse} from '../src/McpResponse.js'; @@ -1064,7 +1067,12 @@ export function createMockContextAnalysisResult(): DevTools.HeapSnapshotModel.He export function createMockParsedArguments( options: Partial = {}, ): ParsedArguments { - const defaultArgs = parser('0.0.0', ['node', 'main.js']).parseSync(); + const defaultArgs = parseArguments( + '0.0.0', + ['node', 'main.js'], + process.env, + false, + ); return {...defaultArgs, ...options}; } From cac095de107ccffb8df76c5f94327b58f0094003 Mon Sep 17 00:00:00 2001 From: Nikolay Vitkov Date: Tue, 29 Sep 2026 12:31:56 +0000 Subject: [PATCH 2/3] fixes --- src/config/browser-options.ts | 1 - src/config/mcp-options.ts | 61 +++++++++++++++++++++-------------- 2 files changed, 36 insertions(+), 26 deletions(-) diff --git a/src/config/browser-options.ts b/src/config/browser-options.ts index d824301d3..9a3bb7483 100644 --- a/src/config/browser-options.ts +++ b/src/config/browser-options.ts @@ -59,7 +59,6 @@ export const browserOptions = { type: 'string', description: 'Custom headers for WebSocket connection in JSON format (e.g., \'{"Authorization":"Bearer token"}\'). Only works with --wsEndpoint.', - implies: 'wsEndpoint', coerce: (val: string | undefined) => { if (!val) { return; diff --git a/src/config/mcp-options.ts b/src/config/mcp-options.ts index 22466d5fb..27fe22935 100644 --- a/src/config/mcp-options.ts +++ b/src/config/mcp-options.ts @@ -119,13 +119,11 @@ export const mcpOptions = { experimentalFfmpegPath: { type: 'string', describe: 'Path to ffmpeg executable for screencast recording.', - implies: 'experimentalScreencast', }, experimentalScreencastFps: { type: 'number', describe: 'Frames per second to use for screencast recording. Lower values can reduce memory pressure on pages that produce frames faster than ffmpeg can encode them.', - implies: 'experimentalScreencast', coerce: (value: number | undefined) => { if (value === undefined) { return; @@ -324,10 +322,16 @@ export const mcpOptions = { }, } satisfies Record; -export type ParsedArguments = ReturnType< +type RawParsedArguments = ReturnType< ReturnType>['parseSync'] >; +export type ParsedArguments = { + [K in keyof RawParsedArguments as K extends '_' | '$0' + ? never + : K]: RawParsedArguments[K]; +}; + export function getMcpOptionsForViaCli(): Record< keyof typeof mcpOptions, YargsOptions @@ -373,9 +377,8 @@ export function getMcpOptionsForViaCli(): Record< export function getCliOptions(): Partial< Record > { - const options: Partial> = { - ...getMcpOptionsForViaCli(), - }; + const options: Partial> = + withoutDefaults(getMcpOptionsForViaCli()); // Missing CLI serialization. delete options.viewport; @@ -384,19 +387,6 @@ export function getCliOptions(): Partial< delete options.experimentalStructuredContent; delete options.experimentalInteropTools; - const recordOptions: Record = options; - for (const [key, option] of Object.entries(recordOptions)) { - if (option?.default !== undefined) { - const copy: YargsOptions = { - ...option, - defaultDescription: - option.defaultDescription ?? JSON.stringify(option.default), - }; - delete copy.default; - recordOptions[key] = copy; - } - } - return options; } @@ -475,6 +465,13 @@ const CONFLICTING_ARGS: Array> = [ ['categoryExtensions', 'browserUrl', 'wsEndpoint'], ]; +const IMPLICATIONS: Array<[keyof typeof mcpOptions, keyof typeof mcpOptions]> = + [ + ['wsHeaders', 'wsEndpoint'], + ['experimentalFfmpegPath', 'experimentalScreencast'], + ['experimentalScreencastFps', 'experimentalScreencast'], + ]; + function isPlainObject(value: unknown): value is Record { return typeof value === 'object' && value !== null && !Array.isArray(value); } @@ -518,12 +515,11 @@ function withoutDefaults( return result; } -function stripYargsPositionalArgs( - parsed: Partial, -): Partial { - delete (parsed as {_?: unknown})._; - delete (parsed as {$0?: unknown}).$0; - return parsed; +function stripYargsPositionalArgs( + parsed: T, +): Omit { + const {_: _positionals, $0: _scriptName, ...rest} = parsed; + return rest; } /** @@ -598,6 +594,20 @@ export function validateConflicts( } } +export function validateImplications( + explicitArgs: Partial, +): void { + for (const [key, implied] of IMPLICATIONS) { + const isKeySet = + explicitArgs[key] !== undefined && explicitArgs[key] !== false; + const isImpliedSet = + explicitArgs[implied] !== undefined && explicitArgs[implied] !== false; + if (isKeySet && !isImpliedSet) { + throw new Error(`Implications failed:\n ${key} -> ${implied}`); + } + } +} + /** * Step 5: fills in defaults for everything that was not set explicitly. * `viaCli` is an explicit input like any other; it selects which defaults @@ -674,6 +684,7 @@ export function parseArguments( const explicitArgs = {...configFileArgs, ...cliArgs}; warnUnknownArgs(cliArgs); validateConflicts(explicitArgs); + validateImplications(explicitArgs); const resolvedArgs = applyDefaults(explicitArgs, env); // The merge and the default loop lose the static type that yargs infers from From 9427090335d627961c54f041049eef167279c2fd Mon Sep 17 00:00:00 2001 From: Nikolay Vitkov Date: Tue, 29 Sep 2026 12:54:51 +0000 Subject: [PATCH 3/3] fix --- src/config/mcp-options.ts | 6 +++--- tests/config/mcp-options.test.ts | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/src/config/mcp-options.ts b/src/config/mcp-options.ts index 27fe22935..671cbebac 100644 --- a/src/config/mcp-options.ts +++ b/src/config/mcp-options.ts @@ -327,9 +327,9 @@ type RawParsedArguments = ReturnType< >; export type ParsedArguments = { - [K in keyof RawParsedArguments as K extends '_' | '$0' - ? never - : K]: RawParsedArguments[K]; + [ + K in keyof RawParsedArguments as K extends '_' | '$0' ? never : K + ]: RawParsedArguments[K]; }; export function getMcpOptionsForViaCli(): Record< diff --git a/tests/config/mcp-options.test.ts b/tests/config/mcp-options.test.ts index e4de84963..8b043edc0 100644 --- a/tests/config/mcp-options.test.ts +++ b/tests/config/mcp-options.test.ts @@ -15,6 +15,7 @@ import { parseCliArgs, parseConfigFile, validateConflicts, + validateImplications, } from '../../src/config/mcp-options.js'; import {createTempFile} from '../utils.js'; @@ -92,6 +93,37 @@ describe('mcp-options steps', () => { /Arguments channel and browserUrl are mutually exclusive/, ); }); + + describe('validateImplications', () => { + it('accepts when implying key is not set', () => { + validateImplications({wsEndpoint: 'ws://localhost:9222'}); + }); + + it('accepts when both implying and implied keys are set', () => { + validateImplications({ + wsHeaders: {Auth: 'token'}, + wsEndpoint: 'ws://localhost:9222', + }); + }); + + it('rejects when implying key is set but implied key is missing', () => { + assert.throws( + () => validateImplications({wsHeaders: {Auth: 'token'}}), + /Implications failed:\n {2}wsHeaders -> wsEndpoint/, + ); + }); + + it('rejects when implying key is set but implied key is negated', () => { + assert.throws( + () => + validateImplications({ + experimentalFfmpegPath: '/bin/ffmpeg', + experimentalScreencast: false, + }), + /Implications failed:\n {2}experimentalFfmpegPath -> experimentalScreencast/, + ); + }); + }); }); describe('applyDefaults', () => { it('keeps explicit values and fills in defaults', () => {