fix: restore bare-expression support in browser_evaluate#126
fix: restore bare-expression support in browser_evaluate#126JustasMonkev wants to merge 3 commits into
Conversation
The playwright-core 1.59 workaround (toString-overridden Function) always treated the source as a function, so expressions like "document.title" failed in the page with "TypeError: result is not a function". Auto-detect function vs expression inside the page, mirroring upstream playwright-mcp: eval the source, call it (with the element for element evaluations) when it is a function, otherwise use its value as the result. The generated Playwright code now reflects the wrapped form for expressions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BWQaMNtHe22ugE7B65QAGJ
The direct harness previously failed its own coverage check on startup (it tested browser_network_request and browser_drop, tools this server never exposed) so `npm run test:mcp` could not run at all. Rework it to: - support multiple named cases per tool with per-case durations, summary.json, and a --grep filter alongside --only - add protocol-level verification: server identity/version/instructions, ping, well-formed tool catalog (descriptions, schemas, annotations), unknown-tool rejection, and zod schema validation errors - add negative and edge cases: unreachable navigation, stale refs, page exceptions, dialog dismiss/prompt text and no-dialog error, find regex/no-match/conflicting args, wait_for textGone and missing condition, multi-select, double click, slow typing with submit, tabs without index - verify real outcomes, not just completion: screenshots are valid PNGs on disk, scan_page detects a known image-alt violation and reports zero on a clean fixture page, audit_site/scan_page_matrix/ audit_keyboard write parseable JSON reports with structuredContent and resource links - retry navigations that race Chromium's error page after an intentionally failed navigation - pass through --browser/--executable-path/--server-arg (and MCP_HARNESS_SERVER_ARGS) so the harness runs where the default Chrome channel is unavailable, plus --list-tools to enumerate exposed tools The Claude CLI loop harness now verifies its prompt file covers every exposed tool before running (browser_find had no prompt and went untested) and gains a --skip-coverage escape hatch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BWQaMNtHe22ugE7B65QAGJ
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR expands the MCP harness with protocol, browser, artifact, reporting, filtering, and coverage validation. It also updates ChangesMCP harness validation
Browser evaluation compatibility
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Harness
participant MCPServer
participant FixturePage
participant Results
CLI->>Harness: provide filters and server arguments
Harness->>MCPServer: start transport and enumerate tools
Harness->>MCPServer: execute selected protocol and browser cases
MCPServer->>FixturePage: navigate and perform interactions
FixturePage-->>MCPServer: return results and report artifacts
MCPServer-->>Harness: return tool responses and errors
Harness->>Results: write case logs and summary files
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/run-mcp-direct-harness.mjs:
- Around line 671-684: The summary generation after the main execution loop must
account for cases skipped by fail-fast. Update the counts in the summary.json
write flow to distinguish unrun/aborted cases or make total reflect executed
cases, using the existing passed, skipped, failed, selected, and caseResults
values while preserving the recorded cases.
- Around line 61-65: Preserve the original tools array in the harness state
instead of relying only on the name-keyed Map, which overwrites duplicates.
Update the duplicate-name validation in the “tool catalog is well-formed” test
to iterate the raw catalog, while keeping the Map for name-based lookups.
In @.claude/run-mcp-tool-loop.sh:
- Around line 107-109: Update the tool-enumeration failure branch in
run-mcp-tool-loop.sh to exit nonzero instead of continuing when --list-tools
fails. Preserve the explicit --skip-coverage path as the only way to bypass the
coverage check.
In `@src/tools/evaluate.ts`:
- Around line 74-80: The generated evaluate actions currently quote the
callback, producing a string instead of executable code. In
src/tools/evaluate.ts lines 74-80, update the evaluate response generation to
emit codeExpression directly for both locator and page evaluation paths; update
tests/tools-evaluate.test.ts lines 79 and 92 to expect the unquoted
page.evaluate(() => ...) output.
In `@tests/tools-evaluate.test.ts`:
- Around line 76-78: Update the tests around the captured callbacks in
tools-evaluate.test.ts to invoke the mocked functions rather than only asserting
their type and source text. Add coverage for simple function, expression, and
fake-element inputs, verifying each callback’s returned behavior and thereby
exercising eval, function invocation, and element-expression handling; follow
nearby Vitest patterns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24851f4b-7a4a-412e-958d-804d90c85c72
⛔ Files ignored due to path filters (1)
.claude/mcp-tool-prompts.tsvis excluded by!**/*.tsv
📒 Files selected for processing (4)
.claude/run-mcp-direct-harness.mjs.claude/run-mcp-tool-loop.shsrc/tools/evaluate.tstests/tools-evaluate.test.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb095097e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const value = eval(`(${expression})`); | ||
| const isFunction = typeof value === 'function'; | ||
| const result = await (isFunction ? value() : value); |
There was a problem hiding this comment.
When the evaluated source references a page global whose name collides with this wrapper's locals, such as result, the direct eval creates the user's function/expression in the callback's lexical scope instead of the page global scope. For example, on a page that sets window.result = 42, browser_evaluate with function: '() => result' now closes over the uninitialized const result here and throws a TDZ error, whereas it should read the page's global value. This makes otherwise valid page evaluations fail for common global names like result, value, or expression.
Useful? React with 👍 / 👎.
| }, params.function); | ||
| } else { | ||
| evalResult = await tab.page.evaluate(async expression => { | ||
| const value = eval(`(${expression})`); |
There was a problem hiding this comment.
When callers provide a bare expression as a normal JavaScript statement, such as document.title; or document.querySelector('button').click();, wrapping it as (${expression}) makes the source syntactically invalid before evaluation. Since this change is meant to restore bare-expression support, these common expression-statement inputs still fail; strip a trailing semicolon or fall back to statement evaluation before parenthesizing.
Useful? React with 👍 / 👎.
- Evaluate the browser_evaluate source with indirect eval in a deferred arrow wrapper so page globals like 'result' are never shadowed by the callback's locals and bare expressions run exactly once. - Strip trailing semicolons so expression statements like 'document.title;' stay syntactically valid inside the wrapper. - Emit the generated page.evaluate() callback unquoted: Playwright does not auto-invoke function-source strings, so the quoted form returned the function instead of running it (verified against real Chromium). - Exercise the captured in-page callbacks in the evaluate tests (function/expression/promise/element/global-scope cases). - Harness: validate duplicate tool names against the raw catalog, report notRun in summary.json for fail-fast runs, and fail closed when tool enumeration fails unless --skip-coverage is passed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011fz6btcG4uvrxRWSrz74sD
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26f77eda5f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const isFunction = typeof value === 'function'; | ||
| const result = await (isFunction ? value() : value); |
There was a problem hiding this comment.
When callers pass a bare expression whose value is a function, typeof value === 'function' does not mean the source itself was intended as a callback. For example, evaluating window.open or document.querySelector('button').click now calls that function with no receiver, which can open a window, mutate the page, or throw Illegal invocation instead of returning/serializing the expression result. The function-vs-expression decision should be based on the source form before executing the expression; the locator branch has the same pattern.
Useful? React with 👍 / 👎.
| const globalEval = eval; | ||
| const value = globalEval(`() => (${expression})`)(); |
There was a problem hiding this comment.
On pages that set a CSP such as script-src 'self' without unsafe-eval, this in-page eval() is blocked before the user expression runs, so even simple calls like browser_evaluate with () => document.title fail with a CSP EvalError. The previous path handed Playwright a function source instead of calling the page's own eval, so this regresses evaluation on many production sites; the locator branch has the same issue.
Useful? React with 👍 / 👎.
| # Keep the prompt file in sync with the tools the server actually exposes: | ||
| # a tool without a prompt would silently go untested. | ||
| if [[ "$CHECK_COVERAGE" -eq 1 && -z "$ONLY_TOOL" ]]; then | ||
| if exposed_tools="$(node "$SCRIPT_DIR/run-mcp-direct-harness.mjs" --list-tools 2>/dev/null)"; then |
There was a problem hiding this comment.
In a clean source checkout, or after npm run clean, this default coverage check fails before any prompts run because it invokes the direct harness, which starts cli.js; cli.js imports generated lib/program.js, and lib/ is not checked in. That regresses the loop from using the already configured Claude MCP server to requiring a local build, so the default script exits with the generic coverage error unless users know to build first or pass --skip-coverage.
Useful? React with 👍 / 👎.
2 participants
The playwright-core 1.59 workaround (toString-overridden Function) always
treated the source as a function, so expressions like "document.title"
failed in the page with "TypeError: result is not a function". Auto-detect
function vs expression inside the page, mirroring upstream playwright-mcp:
eval the source, call it (with the element for element evaluations) when it
is a function, otherwise use its value as the result. The generated
Playwright code now reflects the wrapped form for expressions.
Co-Authored-By: Claude Fable 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01BWQaMNtHe22ugE7B65QAGJ
Summary by CodeRabbit
New Features
Bug Fixes