-
Notifications
You must be signed in to change notification settings - Fork 4.7k
feat(mcp): add headers capability #37583
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.
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
Conversation
docs/src/test-agents-js.md
Outdated
|
|
||
| Seed tests provide a ready-to-use `page` context to bootstrap execution. | ||
|
|
||
| ### Custom headers for multi-tenant testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is for the test generation scenario. Please move this section into https://github.com/microsoft/playwright-mcp/blob/main/README.md
| } | ||
|
|
||
| const invalidHeader = entries.find(([name]) => !name.trim()); | ||
| if (invalidHeader) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Playwright will validate this, no need to duplicate the logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried removing this check, but it turns out Playwright doesn’t reject whitespace-only header names. Chromium throws, but Firefox/WebKit happily accept ' ' : value, and the next navigation crashes our MCP static server with “ParseError: Unexpected whitespace after header value”, killing the session. Keeping the guard here prevents that across all browsers.
See the CI run below: https://github.com/microsoft/playwright/actions/runs/18037953508/job/51329266937?pr=37583#step:3:549
| return; | ||
| } | ||
|
|
||
| const normalizedHeaders = Object.fromEntries(entries.map(([name, value]) => [name.trim(), value])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this normalization makes any difference, drop it?
|
Hi @yury-s 👋, I’ve updated the PR based on your feedback. Could you please take another look when you have some time? The team is very excited for this feature for some of the automations we got in our pipeline. Thanks a lot for your help! |
| return; | ||
| } | ||
|
|
||
| const invalidHeader = entries.find(([name]) => !name.trim()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: ideally this check should be done in the setExtraHTTPHeaders implementation.
tests/mcp/headers.spec.ts
Outdated
| expect(secondRequest.headers['x-tenant-id']).toBe('tenant-123'); | ||
| }); | ||
|
|
||
| test('browser_set_headers applies to all requests from the context', async ({ startClient, server }) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we already have it covered by the tests for context.setExtraHttpHeaders, I'd only test that the headers are sent at all.
|
Tests on Windows seem unrelated but they keep failing restarting the job. Let me try again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix lint errors, otherwise looks good!
Hey, I am not sure what I am doing wrong, but lint is passing for me locally for that file -- would it be possible to retry the job, please? |
You need to rebase on the tip of tree, I believe the type has changed. I will retry the job too. |
Test results for "MCP"13 failed 2622 passed, 108 skipped Merge workflow run. |
This reverts commit 1ca9f66.
…crosoft#37904)" This reverts commit 94037eb.
Summary
headerscapability for Playwright MCPbrowser_set_headerstool that persists custom HTTP headers viasetExtraHTTPHeadersTesting
Closes #37575