Skip to content

Conversation

ogadra
Copy link
Contributor

@ogadra ogadra commented Oct 2, 2025

Problem

  • Screenshot saving failed when an empty string ("") was passed as the filename.
  • The resolver treated the empty string as a path to a directory end and incorrectly flagged it as file path for is outside returning an error response instead of an image.
image

Root Cause

  • The logic for assigning a default filename handled cases where the parameter was omitted, but it did not account for cases where an empty string was explicitly provided.

Fix

  • Treat empty filename as null.
  • Let the existing defaulting path assign the default filename (e.g., page-YYYY-MM-DDTHH-mm-ss-SSSZ.png) in the output directory.

Behavior Changes

  • Before: empty string → error response.
  • After: empty string → screenshot is saved with a default filename, normal success payload.

ogadra and others added 3 commits October 2, 2025 10:26
Adds test case to verify that browser_take_screenshot handles
empty filename argument by generating a default timestamped filename.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
Adds z.preprocess to convert empty string to null in filename validation,
allowing empty filename to fall back to default timestamp-based naming.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@ogadra ogadra marked this pull request as ready for review October 2, 2025 02:16
@ogadra ogadra marked this pull request as draft October 2, 2025 02:21
@ogadra ogadra marked this pull request as ready for review October 2, 2025 03:03
@dgozman dgozman requested a review from yury-s October 2, 2025 15:13
Copy link
Member

@yury-s yury-s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When is it called with an empty string instead of omitting the parameter?

const screenshotSchema = z.object({
type: z.enum(['png', 'jpeg']).default('png').describe('Image format for the screenshot. Default is png.'),
filename: z.string().optional().describe('File name to save the screenshot to. Defaults to `page-{timestamp}.{png|jpeg}` if not specified.'),
filename: z.preprocess(v => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just change ?? to || at line 54:

const fileName = await tab.context.outputFile(params.filename || dateAsFileName(fileType), { origin: 'llm', reason: 'Saving screenshot' }); 

@ogadra
Copy link
Contributor Author

ogadra commented Oct 3, 2025

@yury-s Thanks for reviewing!

I'm using this MCP server with the GPT-5 model via Mastra. When the filename is omitted in the prompt, the model sometimes calls the function with filename: "", which appears to be a model hallucination. While switching models is a possible workaround, I suggest that this edge case be handled within the library for improved robustness.

@ogadra ogadra requested a review from yury-s October 3, 2025 07:48
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.

2 participants