Skip to content

fix(mcp): report malformed JSON config instead of falling back to INI#41909

Merged
dcrousso merged 1 commit into
microsoft:mainfrom
dcrousso:fix-41893
Jul 21, 2026
Merged

fix(mcp): report malformed JSON config instead of falling back to INI#41909
dcrousso merged 1 commit into
microsoft:mainfrom
dcrousso:fix-41893

Conversation

@dcrousso

Copy link
Copy Markdown
Contributor

loadConfig fell back to INI parsing on any JSON error, so a typo'd JSON config (e.g. a trailing comma) was silently accepted as INI: the intended settings were dropped for the defaults and the raw JSON kept as an unused top-level key

surface the original SyntaxError when the input is a JSON object ({) while keeping the INI fallback (including files whose first line is a [section] header) and BOM handling

fixes #41893

@dcrousso
dcrousso requested review from pavelfeldman and yury-s July 21, 2026 19:15
`loadConfig` fell back to INI parsing on any JSON error, so a typo'd JSON config (e.g. a trailing comma) was silently accepted as INI: the intended settings were dropped for the defaults and the raw JSON kept as an unused top-level key

surface the original `SyntaxError` when the input is a JSON object (`{`) while keeping the INI fallback (including files whose first line is a `[section]` header) and BOM handling
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@yury-s yury-s left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's time to make .ini support official or drop it? @pavelfeldman wdyt?

yury-s

This comment was marked as duplicate.

@github-actions

Copy link
Copy Markdown
Contributor

Test results for "MCP"

2 failed
❌ [chrome] › mcp/cli-navigation.spec.ts:56 › goto chrome:// page that closes the tab does not crash the response @mcp-windows-latest-chrome
❌ [msedge] › mcp/cli-core.spec.ts:168 › dialog-accept @mcp-windows-latest-msedge

7790 passed, 1269 skipped


Merge workflow run.

@github-actions

Copy link
Copy Markdown
Contributor

Hi, I'm the Playwright bot and I took a first look at the failing CI.

🟡 Uncertain, but both failures lean hard toward pre-existing flakes

Two Windows-only MCP failures, neither on the config-parsing path this PR touches. One is a known pre-existing flake; the other I can't stamp outright because its only failure in the results DB is this PR's own run.

Details

This PR changes only loadConfig's JSON/INI handling in packages/playwright-core/src/tools/mcp/config.ts (plus tests/mcp/config-resolve.spec.ts). Both failures come from the same test run 29863130236 on Windows, and neither test loads a config file — so the diff can't reach either one.

Pre-existing flake / infra

  • [chrome] › mcp/cli-navigation.spec.ts:56 › goto chrome:// page that closes the tab does not crash the response (@mcp-windows-latest-chrome) — confirmed flake. Across the results DB this test fails 16 of 117 runs (14%) on mcp-windows-latest-chrome and is green on every other bot/OS. Crucially it fails on runs the PR can't be responsible for: main pushes (e.g. 29855614143, 29845175997) and unrelated PRs (move-skills, expect-fn, roll-into-pw-webkit/2336, fix-21484). Same toContain signature every time, only on Windows chrome — a timing flake, not this diff.

Uncertain (almost certainly a flake)

  • [msedge] › mcp/cli-core.spec.ts:168 › dialog-accept (@mcp-windows-latest-msedge) — a toContain mismatch on the alert-dialog output. Passes 116 of 117 runs on this bot and 100% on every other bot/browser; the lone failure in the DB is this PR's own run (29863130236, sha 7faf1c08). Because there's no PR-independent failure to point to, I can't satisfy the "same test failing where the PR can't be responsible" rule and call it a flake outright — but the changed code (JSON/INI config parsing) has nothing to do with clicking a button and accepting a dialog, and this is Windows-msedge only.

Caused by this PR

  • None. No failure lands on the config-loading path the PR changes.

To close out the uncertain one, a green re-run of the current head (08137e37) on mcp-windows-latest-msedge would do it.

Triaged by the Playwright bot - agent run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[MCP]: Malformed JSON config is parsed as INI instead of reporting an error

2 participants