From bd38ba05322acdb49f1f2650655d66e4985ba4f0 Mon Sep 17 00:00:00 2001 From: "Tommy D. Rossi" Date: Wed, 4 Feb 2026 11:23:39 +0100 Subject: [PATCH] feat: implicit return for single expressions using acorn parser - Parse code with acorn to detect single expression statements - Auto-wrap expressions with 'return await (...)' for implicit return - Exclude side-effects: assignments, updates (x++), delete operator - Update skill.md examples to remove console.log (keep await) - Add unit tests for shouldAutoReturn function --- playwriter/package.json | 1 + playwriter/src/executor.ts | 63 ++++++++++++++++++- playwriter/src/executor.unit.test.ts | 90 ++++++++++++++++++++++++++++ playwriter/src/skill.md | 10 ++-- pnpm-lock.yaml | 3 + 5 files changed, 160 insertions(+), 7 deletions(-) create mode 100644 playwriter/src/executor.unit.test.ts diff --git a/playwriter/package.json b/playwriter/package.json index 4074fbc..352aafd 100644 --- a/playwriter/package.json +++ b/playwriter/package.json @@ -46,6 +46,7 @@ "@hono/node-ws": "^1.2.0", "@modelcontextprotocol/sdk": "^1.25.2", "@xmorse/cac": "^6.0.7", + "acorn": "^8.15.0", "async-sema": "^3.1.1", "diff": "^8.0.2", "dom-accessibility-api": "^0.7.1", diff --git a/playwriter/src/executor.ts b/playwriter/src/executor.ts index 0a72f97..d95bd35 100644 --- a/playwriter/src/executor.ts +++ b/playwriter/src/executor.ts @@ -12,6 +12,7 @@ import util from 'node:util' import { createRequire } from 'node:module' import { fileURLToPath } from 'node:url' import vm from 'node:vm' +import * as acorn from 'acorn' import { createSmartDiff } from './diff-utils.js' import { getCdpUrl } from './utils.js' import { waitForPageLoad, WaitForPageLoadOptions, WaitForPageLoadResult } from './wait-for-page-load.js' @@ -61,6 +62,61 @@ const usefulGlobals = { structuredClone, } as const +/** + * Determines if code should be auto-wrapped with `return await (...)`. + * Returns true for single expression statements that aren't assignments. + */ +export function shouldAutoReturn(code: string): boolean { + try { + const ast = acorn.parse(code, { + ecmaVersion: 'latest', + allowAwaitOutsideFunction: true, + allowReturnOutsideFunction: true, + sourceType: 'script', + }) + + // Must be exactly one statement + if (ast.body.length !== 1) { + return false + } + + const stmt = ast.body[0] + + // If it's already a return statement, don't auto-wrap + if (stmt.type === 'ReturnStatement') { + return false + } + + // Must be an ExpressionStatement + if (stmt.type !== 'ExpressionStatement') { + return false + } + + // Don't auto-return side-effect expressions + const expr = stmt.expression + if ( + expr.type === 'AssignmentExpression' || + expr.type === 'UpdateExpression' || + (expr.type === 'UnaryExpression' && (expr as acorn.UnaryExpression).operator === 'delete') + ) { + return false + } + + // Don't auto-return sequence expressions that contain assignments + if (expr.type === 'SequenceExpression') { + const hasAssignment = expr.expressions.some((e: acorn.Expression) => e.type === 'AssignmentExpression') + if (hasAssignment) { + return false + } + } + + return true + } catch { + // Parse failed, don't auto-return + return false + } +} + const EXTENSION_NOT_CONNECTED_ERROR = `The Playwriter Chrome extension is not connected. Make sure you have: 1. Installed the extension: https://chromewebstore.google.com/detail/playwriter-mcp/jfeammnjpkecdekppnclgkkffahnhfhe 2. Clicked the extension icon on a tab to enable it (or refreshed the page if just installed)` @@ -721,8 +777,11 @@ export class PlaywrightExecutor { } const vmContext = vm.createContext(vmContextObj) - const wrappedCode = `(async () => { ${code} })()` - const hasExplicitReturn = /\breturn\b/.test(code) + const autoReturn = shouldAutoReturn(code) + const wrappedCode = autoReturn + ? `(async () => { return await (${code}) })()` + : `(async () => { ${code} })()` + const hasExplicitReturn = autoReturn || /\breturn\b/.test(code) const result = await Promise.race([ vm.runInContext(wrappedCode, vmContext, { timeout, displayErrors: true }), diff --git a/playwriter/src/executor.unit.test.ts b/playwriter/src/executor.unit.test.ts new file mode 100644 index 0000000..3f67c60 --- /dev/null +++ b/playwriter/src/executor.unit.test.ts @@ -0,0 +1,90 @@ +import { describe, it, expect } from 'vitest' +import { shouldAutoReturn } from './executor.js' + +describe('shouldAutoReturn', () => { + it('returns true for simple expressions', () => { + expect(shouldAutoReturn('1 + 2')).toBe(true) + expect(shouldAutoReturn('page.title()')).toBe(true) + expect(shouldAutoReturn('await page.title()')).toBe(true) + expect(shouldAutoReturn('accessibilitySnapshot({ page })')).toBe(true) + expect(shouldAutoReturn('context.pages().map(p => p.url())')).toBe(true) + }) + + it('returns true for awaited expressions', () => { + expect(shouldAutoReturn('await Promise.resolve(42)')).toBe(true) + expect(shouldAutoReturn('await page.goto("https://example.com")')).toBe(true) + }) + + it('returns false for assignment expressions', () => { + expect(shouldAutoReturn('state.page = await context.newPage()')).toBe(false) + expect(shouldAutoReturn('const x = 1')).toBe(false) + expect(shouldAutoReturn('let y = 2')).toBe(false) + expect(shouldAutoReturn('x = 5')).toBe(false) + }) + + it('returns false for update expressions', () => { + expect(shouldAutoReturn('x++')).toBe(false) + expect(shouldAutoReturn('++x')).toBe(false) + expect(shouldAutoReturn('x--')).toBe(false) + expect(shouldAutoReturn('--x')).toBe(false) + }) + + it('returns false for delete expressions', () => { + expect(shouldAutoReturn('delete obj.prop')).toBe(false) + }) + + it('returns false for multiple statements', () => { + expect(shouldAutoReturn('const x = 1; x + 1')).toBe(false) + expect(shouldAutoReturn('await page.click("button"); await page.title()')).toBe(false) + expect(shouldAutoReturn('const a = 1\nconst b = 2')).toBe(false) + }) + + it('returns false for return statements', () => { + expect(shouldAutoReturn('return 5')).toBe(false) + expect(shouldAutoReturn('return await page.title()')).toBe(false) + }) + + it('returns false for non-expression statements', () => { + expect(shouldAutoReturn('if (true) { x }')).toBe(false) + expect(shouldAutoReturn('for (let i = 0; i < 10; i++) {}')).toBe(false) + expect(shouldAutoReturn('function foo() {}')).toBe(false) + expect(shouldAutoReturn('class Foo {}')).toBe(false) + }) + + it('returns false for invalid syntax', () => { + expect(shouldAutoReturn('const')).toBe(false) + expect(shouldAutoReturn('{')).toBe(false) + expect(shouldAutoReturn('await')).toBe(false) + }) + + it('handles edge cases', () => { + // Empty string + expect(shouldAutoReturn('')).toBe(false) + // Just whitespace + expect(shouldAutoReturn(' ')).toBe(false) + // Object literal (parsed as block) + expect(shouldAutoReturn('{ foo: 1 }')).toBe(false) + // Parenthesized object literal (expression) + expect(shouldAutoReturn('({ foo: 1 })')).toBe(true) + // Array literal + expect(shouldAutoReturn('[1, 2, 3]')).toBe(true) + }) + + it('handles sequence expressions with assignments', () => { + // Sequence with assignment should not auto-return + expect(shouldAutoReturn('(x = 1, x + 1)')).toBe(false) + // Sequence without assignment should auto-return + expect(shouldAutoReturn('(1, 2, 3)')).toBe(true) + }) + + it('handles comments in code', () => { + expect(shouldAutoReturn('// comment\npage.title()')).toBe(true) + expect(shouldAutoReturn('page.title() // comment')).toBe(true) + expect(shouldAutoReturn('/* block */ 42')).toBe(true) + }) + + it('handles template literals', () => { + expect(shouldAutoReturn('`hello`')).toBe(true) + expect(shouldAutoReturn('`hello ${name}`')).toBe(true) + }) +}) diff --git a/playwriter/src/skill.md b/playwriter/src/skill.md index ab4003b..813620b 100644 --- a/playwriter/src/skill.md +++ b/playwriter/src/skill.md @@ -63,13 +63,13 @@ playwriter -s 1 -e "state.page = await context.newPage(); await state.page.goto( playwriter -s 1 -e "await page.click('button')" # Get page title -playwriter -s 1 -e "console.log(await page.title())" +playwriter -s 1 -e "await page.title()" # Take a screenshot playwriter -s 1 -e "await page.screenshot({ path: 'screenshot.png', scale: 'css' })" # Get accessibility snapshot -playwriter -s 1 -e "console.log(await accessibilitySnapshot({ page }))" +playwriter -s 1 -e "await accessibilitySnapshot({ page })" ``` **Multiline code:** @@ -167,7 +167,7 @@ Locators (especially ones with `>> nth=`) can change when the page updates. Alwa await page.locator('[id="old-id"]').click(); // element may have changed // GOOD: get fresh snapshot, then immediately use locators from it -console.log(await accessibilitySnapshot({ page, showDiffSinceLastCall: true })); +await accessibilitySnapshot({ page, showDiffSinceLastCall: true }) // Now use the NEW locators from this output ``` @@ -229,7 +229,7 @@ await loginPage.waitForURL('**/callback**'); After any action (click, submit, navigate), verify what happened: ```js -console.log('url:', page.url()); console.log(await accessibilitySnapshot({ page })); +page.url() + '\n' + await accessibilitySnapshot({ page }) ``` For visually complex pages (grids, galleries, dashboards), use `screenshotWithAccessibilityLabels({ page })` instead to understand spatial layout. Label refs are short `eN` strings (e.g. `e3`). @@ -366,7 +366,7 @@ state.targetPage = pages[0]; **List all available pages:** ```js -console.log(context.pages().map(p => p.url())); +context.pages().map(p => p.url()) ``` ## navigation diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 0ad2196..e923f63 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -86,6 +86,9 @@ importers: '@xmorse/cac': specifier: ^6.0.7 version: 6.0.7 + acorn: + specifier: ^8.15.0 + version: 8.15.0 async-sema: specifier: ^3.1.1 version: 3.1.1