-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add client.artifacts with get() and list() methods #113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
shahrear33
commented
Dec 17, 2025
- Add Artifacts class with get() method to retrieve artifacts by session ID and object ID
- Support for different artifact types: img_ (returns Buffer), url_ (returns string URL), vid_ (returns cached file path)
- Add Artifacts class with get() method to retrieve artifacts by session ID and object ID - Support for different artifact types: img_ (returns Buffer), url_ (returns string URL), vid_ (returns cached file path)
Summary of ChangesHello @shahrear33, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new feature by adding an Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new Artifacts client for fetching artifacts like images, URLs, and videos. The implementation is a good step forward, but I've found a few issues that need to be addressed. Most importantly, there is a critical path traversal security vulnerability in the video caching logic. Additionally, I've noted some areas for improvement regarding header handling and type safety. The new unit tests are a good start, but they currently lack coverage for the video artifact functionality, which is the most complex part of this new feature. Please see my detailed comments for suggestions.
| } | ||
| } | ||
|
|
||
| const safeSessionId = sessionId.replace(/-/g, ""); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a path traversal vulnerability here. The sessionId is taken from user parameters and is not sufficiently sanitized before being used in path.join. An attacker could provide a sessionId with ../ sequences to write files outside of the intended VLMRUN_CACHE_DIR. This could lead to overwriting arbitrary files to which the user has write access. You should sanitize sessionId to only allow characters that are safe for a filename by using a whitelist approach.
| const safeSessionId = sessionId.replace(/-/g, ""); | |
| const safeSessionId = sessionId.replace(/[^a-zA-Z0-9]/g, ""); |
| ).rejects.toThrow('Artifacts.list() is not yet implemented'); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test suite is missing coverage for the vid_ artifact type. This is the most complex part of the get method's logic, involving file system access, caching, and parsing the Content-Disposition header. Please add tests to cover these scenarios, including:
- A video is successfully downloaded and cached.
- The correct file path is returned.
- If a file already exists in the cache, it is returned without a new download.
- Correct extraction of the file extension from the
Content-Dispositionheader. - The default
.mp4extension is used when the header is absent.
| ); | ||
|
|
||
| const data = Buffer.from(response.data); | ||
| const headers = response.headers as Record<string, string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type assertion as Record<string, string> is unsafe. Axios response header values can be a string, string[], or undefined. This could lead to runtime errors if a header has multiple values. It's safer to handle this possibility, for instance by using a more accurate type like axios.AxiosResponseHeaders and checking the type of the value before using it.
| const disposition = | ||
| headers["content-disposition"] || headers["Content-Disposition"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Axios normalizes response header keys to lowercase. Accessing headers["Content-Disposition"] is redundant and will likely fail to find the header. You should consistently use the lowercase version content-disposition.
| const disposition = | |
| headers["content-disposition"] || headers["Content-Disposition"]; | |
| const disposition = headers["content-disposition"]; |
| * List artifacts for a session. | ||
| * | ||
| * @param sessionId - Session ID to list artifacts for | ||
| * @throws NotImplementedError - This method is not yet implemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JSDoc indicates that this method throws a NotImplementedError, but the implementation throws a generic Error. To maintain consistency between documentation and behavior, you should update the JSDoc to reflect that an Error is thrown.
| * @throws NotImplementedError - This method is not yet implemented | |
| * @throws {Error} This method is not yet implemented |