fix(mcp): create the parent directory for an explicit output file name - #42494
Open
Rizwan Saleem (theRizwan) wants to merge 1 commit into
Open
fix(mcp): create the parent directory for an explicit output file name#42494Rizwan Saleem (theRizwan) wants to merge 1 commit into
Rizwan Saleem (theRizwan) wants to merge 1 commit into
Conversation
Tools that accept a `filename` resolve it through `Response.resolveClientFile()`, which resolves the name against the client workspace but never creates its parent directory. So `browser_take_screenshot` with `filename: "sub/shot.png"` fails with a raw ENOENT, while the auto-named branch of the same method goes through `outputFile()`, which does create it. Also correct the `filename` description on `browser_take_screenshot` and `browser_pdf_save`. Both said a relative name stays within the output directory; it is resolved against the working directory, which is the behaviour the existing tests pin. Fixes microsoft#42487
Author
|
@microsoft-github-policy-service agree |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #42487
The ENOENT
Every tool that takes a
filenameresolves it throughResponse.resolveClientFile(), and only one of that method's two branches creates the directory it is about to write into:filename→Context.outputFile(), which resolves against the output directory andmkdir -ps the parent;filename→Response.resolveClientFilename()→Context.workspaceFile(), which resolves against the client workspace and creates nothing.So a name containing a subdirectory fails with a raw ENOENT, as reported:
The
mkdiradded here runs afterworkspaceFile()has already applied itscheckFile()allowed-roots check, so it can only create directories under roots the caller was allowed to write to in the first place.resolveClientFilename()itself is left alone, because it is also the read path forbrowser_file_upload,browser_drop,browser_set_storage_stateandbrowser_run_code_unsafe, where creating a directory for a path that does not exist would be wrong.The descriptions
The issue also reports that a relative
filenamedoes not land in--output-dir. That half is what the code intends rather than a bug: an explicit name is resolved against the client workspace (an MCP root,_meta.cwdfor the CLI, otherwise the server's cwd), and three existing tests pin it (screenshot.spec.ts"filename: "output.png"",storage.spec.ts"saves to custom filename",roots.spec.ts"should return relative paths when root is specified").What is wrong is the parameter description on
browser_take_screenshotandbrowser_pdf_save, which tells the model the opposite:This corrects those two strings to describe the actual behaviour. If you would rather keep the description and route relative names into the output directory instead, that is a bigger change and I am happy to redo it that way. I went with the direction the tests already commit to.
Test
browser_take_screenshot (filename with a subdirectory)intests/mcp/screenshot.spec.ts. Onmainit fails with exactly the ENOENT above; with the change it passes.Verified on macOS arm64 (M1 Max): the full
tests/mcpchromium project, 639 passed / 107 skipped / 0 failed, plusnpm run tsc,npx eslinton the touched files andnode utils/lint_tests.js.On the contribution policy
CONTRIBUTING asks that an issue be assigned before a PR goes up, and #42487 is not assigned to anyone. I have commented there asking; close this without ceremony if you would rather own it.