From e8568096579dbae978460a61b037913cc730f2de Mon Sep 17 00:00:00 2001 From: "Tommy D. Rossi" Date: Sat, 28 Feb 2026 10:43:45 +0100 Subject: [PATCH] fix: use AST node offsets to strip trailing semicolons in execute wrapping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Single-expression code ending with `;` (e.g. `await foo();`) was wrapped as `return await (await foo();)` — the semicolon inside parens is a SyntaxError. Refactored `shouldAutoReturn` into `getAutoReturnExpression` which returns the exact expression source via acorn's `expr.start`/`expr.end` offsets, naturally excluding the trailing semicolon. More robust than regex stripping since it uses the parser we already have. Closes #58 --- playwriter/src/executor.ts | 49 ++++++++++++++++++++-------- playwriter/src/executor.unit.test.ts | 48 ++++++++++++++++++++++++++- 2 files changed, 83 insertions(+), 14 deletions(-) diff --git a/playwriter/src/executor.ts b/playwriter/src/executor.ts index 425950c..21b8ecb 100644 --- a/playwriter/src/executor.ts +++ b/playwriter/src/executor.ts @@ -69,10 +69,11 @@ const usefulGlobals = { } as const /** - * Determines if code should be auto-wrapped with `return await (...)`. - * Returns true for single expression statements that aren't assignments. + * Parse code and check if it's a single expression that should be auto-returned. + * Returns the exact expression source (without trailing semicolon) using AST + * node offsets, or null if the code should not be auto-wrapped. See #58. */ -export function shouldAutoReturn(code: string): boolean { +export function getAutoReturnExpression(code: string): string | null { try { const ast = acorn.parse(code, { ecmaVersion: 'latest', @@ -83,19 +84,19 @@ export function shouldAutoReturn(code: string): boolean { // Must be exactly one statement if (ast.body.length !== 1) { - return false + return null } const stmt = ast.body[0] // If it's already a return statement, don't auto-wrap if (stmt.type === 'ReturnStatement') { - return false + return null } // Must be an ExpressionStatement if (stmt.type !== 'ExpressionStatement') { - return false + return null } // Don't auto-return side-effect expressions @@ -105,24 +106,44 @@ export function shouldAutoReturn(code: string): boolean { expr.type === 'UpdateExpression' || (expr.type === 'UnaryExpression' && (expr as acorn.UnaryExpression).operator === 'delete') ) { - return false + return null } // 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 null } } - return true + // Use the expression node's start/end offsets to extract just the expression + // source, excluding any trailing semicolon. This is more robust than regex. + return code.slice(expr.start, expr.end) } catch { // Parse failed, don't auto-return - return false + return null } } +/** Backward-compatible helper: returns true if code should be auto-wrapped. */ +export function shouldAutoReturn(code: string): boolean { + return getAutoReturnExpression(code) !== null +} + +/** + * Wraps user code in an async IIFE for vm execution. + * Uses AST node offsets to extract the expression without trailing semicolons, + * avoiding SyntaxError when embedding inside `return await (...)`. See #58. + */ +export function wrapCode(code: string): string { + const expr = getAutoReturnExpression(code) + if (expr !== null) { + return `(async () => { return await (${expr}) })()` + } + return `(async () => { ${code} })()` +} + 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)` @@ -1061,9 +1082,11 @@ export class PlaywrightExecutor { } const vmContext = vm.createContext(vmContextObj) - const autoReturn = shouldAutoReturn(code) - const wrappedCode = autoReturn ? `(async () => { return await (${code}) })()` : `(async () => { ${code} })()` - const hasExplicitReturn = autoReturn || /\breturn\b/.test(code) + const autoReturnExpr = getAutoReturnExpression(code) + const wrappedCode = autoReturnExpr !== null + ? `(async () => { return await (${autoReturnExpr}) })()` + : `(async () => { ${code} })()` + const hasExplicitReturn = autoReturnExpr !== null || /\breturn\b/.test(code) // Track execution timestamps relative to recording start (seconds). // Used to identify idle gaps that can be sped up in demo videos. diff --git a/playwriter/src/executor.unit.test.ts b/playwriter/src/executor.unit.test.ts index 9f7f28e..88ba63c 100644 --- a/playwriter/src/executor.unit.test.ts +++ b/playwriter/src/executor.unit.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest' -import { shouldAutoReturn } from './executor.js' +import { shouldAutoReturn, wrapCode } from './executor.js' describe('shouldAutoReturn', () => { it('returns true for simple expressions', () => { @@ -88,4 +88,50 @@ describe('shouldAutoReturn', () => { expect(shouldAutoReturn('`hello`')).toBe(true) expect(shouldAutoReturn('`hello ${name}`')).toBe(true) }) + + it('returns true for expressions with trailing semicolons', () => { + expect(shouldAutoReturn('await page.title();')).toBe(true) + expect(shouldAutoReturn('page.title();')).toBe(true) + expect(shouldAutoReturn('snapshot({ page });')).toBe(true) + expect(shouldAutoReturn('await screenshotWithAccessibilityLabels({ page });')).toBe(true) + // trailing whitespace after semicolon + expect(shouldAutoReturn('await foo(); ')).toBe(true) + // comment after semicolon + expect(shouldAutoReturn('await foo(); // comment')).toBe(true) + // double semicolons are two statements, not auto-return + expect(shouldAutoReturn('await foo();;')).toBe(false) + }) +}) + +describe('wrapCode', () => { + it('wraps single expressions with return await', () => { + expect(wrapCode('await page.title()')).toBe( + '(async () => { return await (await page.title()) })()', + ) + }) + + it('strips trailing semicolons to avoid SyntaxError (#58)', () => { + // The bug: trailing semicolons inside return await (...;) produce invalid JS + const wrapped = wrapCode('await screenshotWithAccessibilityLabels({ page });') + expect(wrapped).toBe( + '(async () => { return await (await screenshotWithAccessibilityLabels({ page })) })()', + ) + // Verify it's valid JS by parsing it + expect(() => new Function(wrapped)).not.toThrow() + }) + + it('produces valid JS for trailing semicolons on simple calls', () => { + const wrapped = wrapCode('page.title();') + expect(wrapped).toBe('(async () => { return await (page.title()) })()') + expect(() => new Function(wrapped)).not.toThrow() + }) + + it('wraps multi-statement code without return', () => { + const wrapped = wrapCode('const x = 1; console.log(x);') + expect(wrapped).toBe('(async () => { const x = 1; console.log(x); })()') + }) + + it('wraps assignment expressions without return', () => { + expect(wrapCode('x = 5')).toBe('(async () => { x = 5 })()') + }) })