Visitar URL original
fix: block evaluate_script while a dialog is open by Patrick-SCH03 · Pull Request #3196 · ChromeDevTools/chrome-devtools-mcp · GitHub
Skip to content

fix: block evaluate_script while a dialog is open - #3196

Merged
OrKoN merged 4 commits into
ChromeDevTools:mainfrom
Patrick-SCH03:fix/evaluate-script-dialog-check
Oct 16, 2026
Merged

OrKoN merged 4 commits into
ChromeDevTools:mainfrom
Patrick-SCH03:fix/evaluate-script-dialog-check

Conversation

@Patrick-SCH03

@Patrick-SCH03 Patrick-SCH03 commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #2936

Problem

When a dialog is already open on the page, evaluate_script does not fail fast like other blockedByDialog: true tools. It waits for the CDP protocol timeout (180 s) and holds the tool lock the whole time, so every other tool call is queued behind it.

Root cause: evaluate_script is defined with defineTool (not definePageTool) because it can also target a service worker. ToolHandler runs page.throwIfDialogOpen() only in the page-scoped branch, so the blockedByDialog: true flag on evaluate_script has no effect. It is the only non-page-scoped tool with that flag. The check was originally applied to all tools (#1978) and was later limited to page-scoped tools in #2030, so that non-page tools do not have to resolve a page.

Behavior change

Before this change, evaluate_script could still run while a dialog that was already open and recorded on the page (for example one opened by a timer, as in the last part of #2936) was showing. It now returns A dialog is open (...) in that case, and handle_dialog is the way forward.

Fix

In the page path of the evaluate_script handler, call mcpPage.throwIfDialogOpen() after the target page is resolved and before any evaluation. blockedByDialog is now false on evaluate_script, because the flag has no effect on a defineTool tool, and ToolDefinition and BrowserlessToolDefinition narrow blockedByDialog to false, so the compiler rejects true on any non-page tool (PageToolDefinition keeps boolean). The call now fails with the same A dialog is open (<type>: <message>). error as other blocked tools.

  • ToolHandler is unchanged, so non-page tools still do not need a page (the fix: only require a page in page-scoped tools #2030 behavior is kept). A generic check there would also have to tell the page path from the serviceWorkerId path, and would block worker evaluations that a page dialog does not affect, so the check lives in the handler where the target page is already resolved.
  • The service worker path is unchanged, because a page dialog does not block a worker.
  • The error response is the bare A dialog is open (...) error. Unlike page-scoped tools, it does not include the # Open dialog ... Call handle_dialog hint, because ToolHandler only attaches the page to the response for page-scoped tools. A TODO in script.ts tracks adding the hint once Response can attach the page for non-page-scoped tools.
  • throwIfDialogOpen() ignores dialogs that were already handled (fix: don't throw if Dialog was handled #2437), so a dialog that evaluate_script auto-handled through dialogAction does not block later calls.
  • A dialog that opens during the script is still handled per dialogAction.

Tests

  • tests/tools/script.test.ts: a mock-based handler test. When throwIfDialogOpen throws, the handler rejects with that error and never calls waitForEventsAfterAction.
  • tests/index.test.ts (e2e, Dialogs): a click opens an alert, then evaluate_script returns isError with A dialog is open (alert: test dialog)..

Verification

  • With the issue's scenario against a local build, main returned Runtime.evaluate timed out ... after 180008 ms. With this change it returns Error: A dialog is open (alert: hello alert). in 4 ms.
  • Both new tests fail when the fix is reverted. The e2e test hits a client request timeout, and the handler test reports Missing expected rejection.
  • tests/ToolHandler.test.ts, tests/tools/script.test.ts, tests/McpPage.test.ts and tests/index.test.ts pass, along with npm run build, eslint and prettier on the changed files.

evaluate_script is defined with defineTool, so ToolHandler skips the
blockedByDialog check that runs only for page-scoped tools. With a dialog
already open, the evaluation waited for the 180 s protocol timeout while
holding the tool lock. Check the resolved page before evaluating so the
call fails fast with the same "A dialog is open" error as other blocked
tools.

Fixes ChromeDevTools#2936
@OrKoN
OrKoN self-requested a review October 16, 2026 09:04

@OrKoN OrKoN left a comment

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.

Please check the following:

  1. In src/tools/script.ts, set blockedByDialog: false on evaluate_script, because the flag has no effect on a defineTool tool and the new comment under it says so. A true value next to a manual throwIfDialogOpen() call misleads readers and hides the root cause of #2936.
  2. In src/tools/ToolDefinition.ts, narrow blockedByDialog to the literal type false in ToolDefinition (keep boolean in PageToolDefinition). The compiler then rejects a true flag on any non-page tool, so the same silent gap cannot return.

Nits

  • The PR description must list the behavior change: before this change, evaluate_script could run while a timer-opened dialog was open and recorded (see the last paragraph of #2936); it now returns an error in that case and handle_dialog is the only path forward.
  • The error text does not include the # Open dialog ... Call handle_dialog hint, because Response does not expose setPage. Add a short follow-up issue or TODO so the hint is not lost.
  • tests/index.test.ts: the new e2e test duplicates the handle_dialog round-trip that other Dialogs tests already cover. The mock-based handler test proves the fast-fail path; the e2e test can stop after the first isError assertion.
  • docs/tool-reference.md needs no update: scripts/generate-docs.ts does not print blockedByDialog.

blockedByDialog has no effect on tools defined with defineTool, so set it
to false on evaluate_script and narrow the type to false in
ToolDefinition, making the compiler reject true on any non-page tool.
Add a TODO for the missing handle_dialog hint, and trim the e2e test to
the fast-fail assertion since other Dialogs tests cover handle_dialog.
BrowserlessToolDefinition, added on main, has no page either, so narrow
its blockedByDialog to false as well.
@Patrick-SCH03

Copy link
Copy Markdown
Contributor Author

Thanks for the review! Addressed:

  1. evaluate_script now sets blockedByDialog: false (3958071).
  2. ToolDefinition narrows blockedByDialog to false, and PageToolDefinition keeps boolean. Setting true on evaluate_script is now a compile error. After merging main, I narrowed the new BrowserlessToolDefinition the same way, since it has no page either (d18ebd2).
  • The PR description now has a behavior change section: a dialog that is already open and recorded, for example one opened by a timer, now makes evaluate_script return the error, and handle_dialog is the way forward.
  • Added a TODO in script.ts for the # Open dialog ... Call handle_dialog hint, to add once Response can attach the page for non-page-scoped tools.
  • The e2e test now stops after the first isError assertion.

tests/tools/script.test.ts, tests/ToolHandler.test.ts and the Dialogs e2e tests pass on the merged branch.

@OrKoN
OrKoN self-requested a review October 16, 2026 09:44
@OrKoN
OrKoN added this pull request to the merge queue Oct 16, 2026
Merged via the queue into ChromeDevTools:main with commit 545bcbb Oct 16, 2026
15 checks passed
zjdCander pushed a commit to zjdCander/chrome-devtools-mcp that referenced this pull request Oct 19, 2026
## Summary

Add a **Pre-submit checklist** section to `AGENTS.md`. Each item comes
from a recent review finding or a landed bug fix in this repository.

The checklist has five groups:

- **Scope and PR description**: one change per PR, title and commit type
match, description lists every behavior change and matches the diff,
`npm run gen` and `npm run format` ran.
- **Design**: fix in the lowest layer that owns the data, fix upstream
(Puppeteer, devtools-frontend) first, one helper for repeated code,
options objects instead of positional `undefined` arguments, validation
in Zod schemas.
- **Correctness and edge cases**: no stale state after a failed call,
artifacts written before a throw, abandoned async continuations, mutex
guards, file removal races, `dispose()` releases every resource, secrets
never reach logs, path parameters validated even when they parse as
URLs.
- **Tests**: every new function has a test, no vacuous assertions,
`notCalled` targets the first call after validation, exact-argument
assertions, reusable stubs in `tests/mocks.ts`, bounded timeouts, no
second CDP session for event waits.
- **Documentation and output**: tool descriptions state when a parameter
is ignored, skills stay generic, one-sentence inline comments.

Sources: review comments on ChromeDevTools#2679, ChromeDevTools#2797, ChromeDevTools#2859, ChromeDevTools#2881, ChromeDevTools#2901, ChromeDevTools#3048,
ChromeDevTools#3082, ChromeDevTools#3109, ChromeDevTools#3120, ChromeDevTools#3132, ChromeDevTools#3134, ChromeDevTools#3139, ChromeDevTools#3148, ChromeDevTools#3156, ChromeDevTools#3158, ChromeDevTools#3161,
ChromeDevTools#3182, ChromeDevTools#3183, ChromeDevTools#3186, ChromeDevTools#3187, ChromeDevTools#3188, ChromeDevTools#3189, ChromeDevTools#3192, ChromeDevTools#3196, ChromeDevTools#3197, ChromeDevTools#3198,
ChromeDevTools#3203, ChromeDevTools#3204, and fixes ChromeDevTools#3150, ChromeDevTools#3160, ChromeDevTools#3185, ChromeDevTools#3202, ChromeDevTools#3207, ChromeDevTools#3208.

Existing content in `AGENTS.md` is unchanged.

Closes ChromeDevTools#3222
github-merge-queue Bot pushed a commit that referenced this pull request Oct 22, 2026
🤖 I have created a release *beep* *boop*
---


##
[1.12.0](chrome-devtools-mcp-v1.11.0...chrome-devtools-mcp-v1.12.0)
(2026-10-22)


### 🎉 Features

* add --maxNavigationSaved flag to configure retained navigation history
([#3018](#3018))
([a580d13](a580d13))
* add Kimi Code plugin manifest
([#3029](#3029))
([cd00b64](cd00b64))
* Analyze heap snapshots without a browser connection
([#3156](#3156))
([78c8320](78c8320))
* **config:** add --watchConfig to reload config file changes
([#3248](#3248))
([b48561f](b48561f))
* **config:** introduce layered ServerConfig for live reload
([#3203](#3203))
([37f1fda](37f1fda))
* **config:** share ReadonlyServerConfig with McpContext and McpPage
([#3204](#3204))
([0813bf3](0813bf3))
* **config:** support reloading blockedUrlPattern and allowedUrlPattern
([#3268](#3268))
([cc850f5](cc850f5))
* **context:** release disabled feature state on config reload
([#3049](#3049))
([c0140d1](c0140d1))
* Paginate by default in heap snapshot tools
([#3201](#3201))
([fb8375b](fb8375b))
* Paginate detailed diff in compare_heapsnapshots
([#3195](#3195))
([4a9dd53](4a9dd53))
* run independent tool calls in parallel with a TaskRunner
([#3243](#3243))
([8a8d877](8a8d877))
* run page-scoped tools on different pages in parallel
([#3252](#3252))
([f8f18ac](f8f18ac))
* **server:** add McpServer.applyConfig() to reload configuration
([#3051](#3051))
([f874aeb](f874aeb))
* **slim:** support filepath in slim mode screenshot
([#3296](#3296))
([d399c4a](d399c4a))
* Sort objects for class by self size in compare_heapsnapshots
([#3205](#3205))
([f8a18c6](f8a18c6))
* **tools:** support updating ToolHandler definitions in place
([#3050](#3050))
([8064138](8064138))
* **wait_for:** support opt-in visible text matching
([#3212](#3212))
([eef92da](eef92da))


### 🛠️ Fixes

* allow paths below a filesystem-root workspace root
([#3277](#3277))
([296ccef](296ccef))
* block evaluate_script while a dialog is open
([#3196](#3196))
([545bcbb](545bcbb))
* bound take_snapshot wait and clear the geolocation override
([#3265](#3265))
([6282f54](6282f54))
* **browser:** redact WebSocket headers from connection logs
([#2901](#2901))
([3a2f91e](3a2f91e))
* capture thrown transform errors as Zod validation issues
([#3242](#3242))
([0f95c21](0f95c21))
* clear handled dialog in McpPage.getDialog()
([#3241](#3241))
([2ab93a3](2ab93a3)),
closes
[#2958](#2958)
* **cli:** disallow --slim in CLI start options
([#2987](#2987))
([#3113](#3113))
([ae3f61c](ae3f61c))
* **cli:** do not forward tool args to start
([#3114](#3114))
([fef1598](fef1598))
* **cli:** serialize object daemon arguments
([#2797](#2797))
([f73a440](f73a440))
* close the tab opened by a failed new_page
([#2928](#2928))
([c174677](c174677))
* **config:** accept native JSON objects for wsHeaders in config files
([#3262](#3262))
([371e6c2](371e6c2))
* **console:** describe Map, Set, function, DOM node, symbol and cyclic
arguments
([#3199](#3199))
([968e4e5](968e4e5))
* **console:** include the reason of unhandled promise rejections
([#3148](#3148))
([f7cdf73](f7cdf73))
* defer focus emulation until a page is explicitly targeted
([#3230](#3230))
([35abe35](35abe35))
* disable Puppeteer signal handlers for launched browsers
([#3238](#3238))
([3afec73](3afec73)),
closes
[#3154](#3154)
* discover third-party developer tools before tool lookup
([#3208](#3208))
([2777fa0](2777fa0)),
closes
[#2973](#2973)
* discover third-party developer tools in page tools
([#3207](#3207))
([cf33529](cf33529)),
closes
[#2980](#2980)
* drop InlineTextBox nodes from text snapshots
([#3202](#3202))
([f8c775a](f8c775a))
* **emulate:** keep previous viewport when setViewport fails
([#3150](#3150))
([19d883c](19d883c))
* exclude source maps from the npm package
([#3295](#3295))
([d575da4](d575da4))
* keep surrogate pairs and emoji whole when truncating titles, names and
bodies
([#3215](#3215))
([058adcf](058adcf))
* let resourceTypes match CORS preflights and prefetches
([#3217](#3217))
([e342c6e](e342c6e))
* **lighthouse:** explain unsupported URL schemes in navigation mode
([#3144](#3144))
([c6ad84a](c6ad84a))
* **lighthouse:** fail the audit when Lighthouse reports a runtime error
([#3188](#3188))
([53ea78d](53ea78d))
* **lighthouse:** restore the emulated viewport after a navigation audit
([#3189](#3189))
([c777ece](c777ece))
* **network:** list only earlier hops in a request's redirect chain
([#3198](#3198))
([4675940](4675940))
* **network:** remove unsupported websocket from list_network_requests
filter
([#2961](#2961))
([#3282](#3282))
([8b119d9](8b119d9))
* **network:** reset page network emulation before applying URL
restrictions
([#3312](#3312))
([f25f3df](f25f3df))
* **pages:** reject devtools:// URLs in navigate_page and new_page
([#2935](#2935))
([#3128](#3128))
([c94c03d](c94c03d))
* **pages:** report the actual page size after resize_page
([#2998](#2998))
([#3132](#3132))
([f003056](f003056))
* **pages:** revert reporting the page size after resize_page
([#3308](#3308))
([186ea66](186ea66)),
closes
[#3306](#3306)
* **pagination:** return all items when pageSize is omitted and pageIdx
is 0
([#2996](#2996))
([#3134](#3134))
([3ad9115](3ad9115))
* **performance:** populate structuredContent for traceInsight in
performance_analyze_insight
([#2981](#2981))
([#3115](#3115))
([c2c6cc4](c2c6cc4))
* preserve unread page fallback across snapshots and deduplicate page
disposal
([#3161](#3161))
([8604365](8604365))
* print the no-roots temp-directory warning under default flags
([#2927](#2927))
([0dd8ed9](0dd8ed9))
* reject empty, extra, and unknown parts in geolocation and viewport
transforms
([#3097](#3097))
([#3120](#3120))
([3ea31fb](3ea31fb))
* resolve error causes on the session that reported the exception
([#3220](#3220))
([b2d37e8](b2d37e8))
* resolve service worker IDs for console messages
([#2881](#2881))
([3804a9a](3804a9a))
* return message when page exposes no 3p developer tools
([#3197](#3197))
([663f3c3](663f3c3)),
closes
[#3004](#3004)
* **script:** surface non-Error thrown and rejected values in
evaluate_script
([#2951](#2951))
([#3127](#3127))
([b1e6e68](b1e6e68))
* **security:** validate filesystem paths that parse as http/ws URLs
([#3186](#3186))
([91e112d](91e112d))
* **snapshot:** omit uid for accessibility nodes without backendNodeId
([#3187](#3187))
([9054351](9054351))
* stop active screencast on McpPage.dispose()
([#3160](#3160))
([558af81](558af81))
* stop get_network_request waiting forever for an unread no-store body
([#3216](#3216))
([c556be1](c556be1))
* surface nested error causes and autoConnect hint on connection failure
([#3111](#3111))
([d7f1e78](d7f1e78))
* treat 0.0.0.0 and [::] browser endpoints as local
([#3321](#3321))
([7d5f7de](7d5f7de))
* treat IPv4-mapped IPv6 loopback as localhost
([#3255](#3255))
([6cfb34c](6cfb34c))
* truncate data: URLs in accessibility snapshots
([#3292](#3292))
([11e7d47](11e7d47))
* validate type_text submitKey before typing
([#3185](#3185))
([c8615d7](c8615d7))
* **wait_for:** find text in dynamically attached iframes
([#3210](#3210))
([5f84888](5f84888))
* warn when screenshot dimensions exceed 16384px limit
([#2952](#2952))
([#3291](#3291))
([10455b7](10455b7))


### 📄 Documentation

* add a pre-submit checklist to AGENTS.md
([#3236](#3236))
([e96ee6c](e96ee6c))
* clarify URL pattern port and hostname matching
([#3287](#3287))
([4723335](4723335))
* **config:** note that hot-reloaded URL restrictions keep open targets
attached
([#3314](#3314))
([47db2b8](47db2b8))
* correct CLI availability of extension tools
([#2852](#2852))
([a7280ee](a7280ee))
* explain how to install skills, including the CLI skill
([#3274](#3274))
([46eb546](46eb546))
* remove experimental status from CLI and promote it in README
([#3273](#3273))
([0ced16b](0ced16b))
* require a rebase only when the branch has merge conflicts
([#3239](#3239))
([d1143ac](d1143ac))
* **skills:** clarify Lighthouse score scale and contrast workflow
([#3006](#3006))
([#3218](#3218))
([bf8c6d9](bf8c6d9))


### ⚡ Performance

* **cli:** load a small third-party bundle in the chrome-devtools CLI
([#3200](#3200))
([552e1f8](552e1f8))
* load the Lighthouse bundle on first use
([#3169](#3169))
([798f03a](798f03a))


### 🏗️ Refactor

* **config:** isolate scripts and unit tests from home config discovery
([#3256](#3256))
([65e6b97](65e6b97))
* **config:** lock slim and pageIdRouting options when viaCli is set
([#3257](#3257))
([384d9aa](384d9aa))
* **config:** pass ReadonlyServerConfig to BrowserManager and add
OptionGate
([#3267](#3267))
([98302f2](98302f2))
* **config:** simplify watchConfig and start watcher inside connect
([#3259](#3259))
([878c5f6](878c5f6))
* extract errorResult() and routedPageId() in ToolHandler
([#3251](#3251))
([9a6b03a](9a6b03a))
* import types and values from a module in one statement
([#3303](#3303))
([f4eaf1f](f4eaf1f))
* **lighthouse:** move Lighthouse runner methods to McpPage and add
bundle types
([#3283](#3283))
([e1823a2](e1823a2))
* list optional properties last in inline object types
([#3302](#3302))
([dc929c0](dc929c0))
* register all tools upfront across modes
([#3163](#3163))
([58f8d1f](58f8d1f))
* **response:** pass live ReadonlyServerConfig to McpResponse
([#3258](#3258))
([6042490](6042490))
* **tools:** add explicit factory types to tool definitions
([#3284](#3284))
([4d1d7b2](4d1d7b2))
* use #private names instead of the private modifier
([#3301](#3301))
([2e13311](2e13311))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
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.

evaluate_script hangs for the protocol timeout when a dialog is already open

2 participants